Cleanup fallthrough in FilePermission and PropertyPermission was: Request for Review of 7116890 (Warning Cleanup java.io)
Sebastian Sickelmann
sebastian.sickelmann at gmx.de
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
> java.io.FilePermission
> 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/FilePermission.java
> 2. java/net/SocketPermission.java
> 3. java/util/PropertyPermission.java
> 4. javax/management/MBeanPermission.java
> 5. javax/security/auth/kerberos/ServicePermission.java
> 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 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
>>
More information about the core-libs-dev
mailing list