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

[Stuart Marks]

I'm adding Weijun (Max) Wang to this thread.

The same "ackbarfaccept" code had come up a third time when I was reviewing 
some of Max's changes. The code in question all has to do with permissions, and 
Max is in the security group, so he might have a better insight whether doing a 
refactoring is appropriate and how to approach doing it.

Some searching around reveals that "ackbarfaccept" appears in the 
PlayerPermission class of the Java ME Mobile Media API (not in any of the 
OpenJDK repositories) so it looks like the code has been copied even more times 
than are visible here. Perhaps a public permissions parsing utility method is 
called for.

s'marks

On 12/2/11 9:02 AM, Sebastian Sickelmann wrote:
> 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?