Request for Review of 7116890 (Warning Cleanup java.io)

[Sebastian Sickelmann]

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