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

Stuart Marks stuart.marks at oracle.com
Wed Aug 29 23:20:03 UTC 2012

On 8/29/12 4:36 AM, Ulf Zibis wrote:

436 switch (actions) { 437 case SecurityConstants.FILEREADACTION: 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

More information about the core-libs-dev mailing list