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

[Jonathan Gibbons]

On 12/01/2011 08: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



Sebastian,

I would expect you are still getting warnings, which can easily be 
fixed, from the use of raw types in ExpiringCache.java, lines 67,68.

-- Jon