Cleanup fallthrough in FilePermission and PropertyPermission was: Request for Review of 7116890 (Warning Cleanup java.io)

Fri Dec 2 06:18:11 UTC 2011

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 FilePermission.java also 
> exists in java.util.PropertyPermission.java.  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 
> FilePermission.java to PropertyPermission.java?  Or, maybe you can 
> apply the same changes yourself to PropertyPermission.java?
>
> 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 ExpiringCache.java 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 FilePermission.java 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] 
>>> http://mail.openjdk.java.net/pipermail/jdk8-dev/2011-December/000380.html
>>>
>>>
>>> On 11/30/11 8:33 PM, Sebastian Sickelmann wrote:
>>>> Hi,
>>>>
>>>> i have a webrev[0] that contains some warning cleanup for java.io
>>>> It is based on rev 7795c41ed54c 
>>>> http://hg.openjdk.java.net/jdk8/tl/jdk/
>>>>
>>>> Some comments to the changes:
>>>>
>>>> ExpiringCache.java: 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.
>>>>
>>>> FilePermission.java: 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]
>>>> http://dl.dropbox.com/u/43692695/oss-patches/openjdk8/Warning_Cleanup_Java_io/webrev0_based_on_7795c41ed54c/index.html 
>>>>
>>>>
>>>>
>> 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] 
>> http://dl.dropbox.com/u/43692695/oss-patches/openjdk8/Warning_Cleanup_Java_io/CR7116890_0/index.html
>>
>> -- Sebastian
>
> -- 
> Oracle <http://www.oracle.com>
> Brandon Passanisi | Principle Member of Technical Staff
>
> Oracle Java Standards Conformance
>
> Green Oracle <http://www.oracle.com/commitment> Oracle is committed to 
> developing practices and products that help protect the environment