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

[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
>
>