Review Request: 7193406 - Clean-up JDK Build Warnings in java.util, java.io

[Stuart Marks]

On 8/29/12 4:36 AM, Ulf Zibis wrote:
>>>  436         switch (actions) {
>>>  437             case SecurityConstants.FILE_READ_ACTION:
>>>  438                 return READ;
>
> Oops, you have reverted the change to use switch-on-Strings by webrev.03. Why?

A fair question. I had instigated some internal conversation about reverting 
this change, so here's the explanation. (Good catch, by the way.)

The original code was like this:

  427     private static int getMask(String actions) {
              ...
  435         // Check against use of constants (used heavily within the JDK)
  436         if (actions == SecurityConstants.FILE_READ_ACTION) {
  437             return READ;
  438         } else if (actions == SecurityConstants.FILE_WRITE_ACTION) {

The various SecurityConstants being used here are Strings.

Note that this is doing String comparisons using == which is usually a bug. But 
this code is relying on String interning to provide object identity for equal 
string literals, in order to provide a fast path optimization for these common 
cases. Changing this code to use switch-over-strings would be semantically 
equivalent, but it would probably slow things down, since switch provides 
equals() semantics instead of == semantics.

The permissions code is quite performance-sensitive, so I've asked Dan to 
revert this change.

>> @SuppressWarnings("fallthrough") is put to suppress warnings generated by
>> another switch/case statements
> Can't you move it from method scope to there?
>
>>             while (i >= matchlen && !seencomma) {
>>                 switch(a[i-matchlen]) {
>>                 case ',':
>>                     seencomma = true;
>>                     /*FALLTHROUGH*/
>>                 case ' ': case '\r': case '\n':
>>                 case '\f': case '\t':
>>                     break;
>>                 default:
>>                     throw new IllegalArgumentException(
>>                             "invalid permission: " + actions);
>>                 }
>>                 i--;
>>             }

Unfortunately there is no suitable place to put the annotation. Annotations can 
only be placed on declarations, and the narrowest enclosing declaration around 
this switch statement is, unfortunately, the entire method. One might consider 
refactoring the switch statement into its own method, but that kind of 
refactoring is too large a change for warnings cleanup.

s'marks