Cleanup fallthrough in FilePermission and PropertyPermission was: Request for Review of 7116890 (Warning Cleanup

Sebastian Sickelmann sebastian.sickelmann at
Fri Dec 2 17:02:58 UTC 2011

Am 02.12.2011 16:27, schrieb Brandon Passanisi:
> Hi Sebastian.  I'm not sure if you had seen the e-mail from Stuart 
> Marks regarding this, but Stuart was able to find more instances of 
> the similar block of "fallthrough" code.  I can volunteer to apply 
> your upcoming change to FilePermission to the other files if you 
> wish.  Or, you can try applying the change to the other files, but if 
> you don't have time I don't mind doing it.  Here's the section of 
> Stuart's e-mail on this topic:
>     -------------------------------------------------------------------------------------
>     (Incidentally, this is the third time I've reviewed code today that
>     looks exactly like this. The other cases are in
>     and java.util.PropertyPermission. They each have the /*FALLTHROUGH*/
>     into a set of cases that do nothing but break; and they have similar
>     ("ackbarfaccept") comments. It would be nice if these chunks of code
>     could be unified, but they differ in a number of fiddly details.)
>     (The string "ackbarfaccept" occurs in the following files:
>     1. java/io/
>     2. java/net/
>     3. java/util/
>     4. javax/management/
>     5. javax/security/auth/kerberos/
>     Hmmm.)
>     -------------------------------------------------------------------------------------
> Thanks.

Hi Brandon,
Hi Stuart,

i had a look at all those classes and it seems to be the same algorithm. 
In an normal project (not the jdk) i would suggest to completely 
refactor it and make some code de-duplication cleanup. But if i thinks 
on projects like jigsaw this can easily get a real problem. What do you 
think, should we try to cleanup them all. Or should we try to make some 
de-duplication/code-reuse refactoring.

-- Sebastian
> On 12/1/2011 10:18 PM, Sebastian Sickelmann wrote:
>> Hi Brandon,
>> i will try to work out a fix for both and cc the review request to you.
>> -- Sebastian
>> Am 01.12.2011 23:54, schrieb Brandon Passanisi:
>>> Hi Sebastian.  I was speaking with Stuart Marks earlier today and he 
>>> mentioned that the "fallthrough" code in also 
>>> exists in  Maybe the code author 
>>> had done some copy/paste when working on these files.  Stuart had 
>>> said that you might be planning on doing some work on this after the 
>>> warnings cleanup work.  If/when you do this work, maybe you can let 
>>> me know so that I can sync the same changes you apply to 
>>> to  Or, maybe you can 
>>> apply the same changes yourself to
>>> Thanks.
>>> On 12/1/2011 8:27 AM, Sebastian Sickelmann wrote:
>>>> Am 01.12.2011 10:16, schrieb Stuart Marks:
>>>>> Hi Sebastian,
>>>>> Thanks for submitting this patch! I've filed bug 7116890 to cover 
>>>>> this set of changes.
>>>>> Regarding webrev, it probably does get confused by merges in the 
>>>>> history. By default it tries to show differences between the tip 
>>>>> of the target repository and tip + uncommitted changes in your 
>>>>> local repository. This isn't always the right thing. One thing you 
>>>>> might try is to use the -r option to specify a particular revision 
>>>>> in the history against which to generate the diffs. But, having a 
>>>>> few extra files isn't a big deal.
>>>>> On to your changes.
>>>>> Most of them are really good and are exactly the kind of simple 
>>>>> change we're looking for, as I posted a little while ago. [1]
>>>>> The case is an interesting one. I think it's 
>>>>> possible to add a static serialVersionUID field within the 
>>>>> anonymous class. It's hard to run serialver over an anonymous 
>>>>> class (actually it might be possible to run the checksum algorithm 
>>>>> on the loaded class) but if in practice it's never serialized, who 
>>>>> cares what the "correct" value is? You could just use a value of 0L.
>>>>> An alternative would also be to use @SuppressWarnings("serial"). 
>>>>> Since the constructor is so short, you could just put it on the 
>>>>> constructor.
>>>>> Actually I prefer @SuppressWarnings, since adding serialVersionUID 
>>>>> adds to the system's footprint. If it's never used, we shouldn't 
>>>>> bother defining it.
>>>>> Given these alternatives, it's not necessary to create a static 
>>>>> CacheHashMapImpl class.
>>>>> Regarding the change, yes, I see that the 
>>>>> skipBeforePreviousComma() change is problematic. I puzzled over it 
>>>>> for a while too. I wish I had sent out the Coding Guideline [1] 
>>>>> earlier; it might have saved you an hour. :-) Sorry about that.
>>>>> A cleanup/rewrite of this code would indeed be better done 
>>>>> separate from Warnings Cleanup Day. But I think there might be a 
>>>>> simple way to get rid of the warning without doing a cleanup or 
>>>>> refactoring. The warning message in question is at line 535 of the 
>>>>> original code, about falling through to the next case. But the 
>>>>> next case doesn't actually do anything but break. Could we just 
>>>>> change the /*FALLTHROUGH*/ comment to a break statement? I think 
>>>>> that would fix the warning without changing the logic at all.
>>>>> s'marks
>>>>> [1] 
>>>>> On 11/30/11 8:33 PM, Sebastian Sickelmann wrote:
>>>>>> Hi,
>>>>>> i have a webrev[0] that contains some warning cleanup for
>>>>>> It is based on rev 7795c41ed54c 
>>>>>> Some comments to the changes:
>>>>>> Changed anonymous inner class to inner class 
>>>>>> with the
>>>>>> intention to put serialversion inside of it. But serialver 
>>>>>> doesn't want to give
>>>>>> my the serialver. I also think that ExpiringCache is not 
>>>>>> serializable but the
>>>>>> warning was clear: the anonymous inner class is seriallizable and 
>>>>>> has no
>>>>>> explicit serialversionuid.
>>>>>> I have starred at the code between line 453 
>>>>>> and 547 for
>>>>>> over an hour, because i thought that there is a bug within the 
>>>>>> expression "i >=
>>>>>> matchlen" in line 530 and the both "i != -1" in lines 457 and 
>>>>>> 461. But there is
>>>>>> no bug. But i wanted to left this code slightly more readable. I 
>>>>>> introduced the
>>>>>> method skipBeforePreviousComma to make it possible to work-around 
>>>>>> the
>>>>>> fallthought warning with an return statement. This code-change 
>>>>>> need's some more
>>>>>> review attention. Maybe we should split this up for another 
>>>>>> cleanup. I think
>>>>>> the whole method needs some rewrite.
>>>>>> Some classes had no change at all. Maybe webrev created them 
>>>>>> because there
>>>>>> where changes in my history/branches. There were some patches 
>>>>>> from alan i saw
>>>>>> to late. Maybe webrev is confused of the multiple merges.
>>>>>> Can someone please create a CR for this and
>>>>>> [0]
>>>> Thanks for the good feedback.
>>>> I splitted my change to the trivial part and will start discussion 
>>>> of FilePermission change on core-libs-dev after the cleanup event.
>>>> I created a new webrev with the suggested changes here[2]
>>>> [2] 
>>>> -- Sebastian
>>> -- 
>>> Oracle <>
>>> Brandon Passanisi | Principle Member of Technical Staff
>>> Oracle Java Standards Conformance
>>> Green Oracle <> Oracle is committed 
>>> to developing practices and products that help protect the environment

More information about the core-libs-dev mailing list