On 8/29/2012 7:07 PM, Stuart Marks wrote:
On 8/29/12 4:56 PM, Joe Darcy wrote:
On 8/29/2012 4:20 PM, Stuart Marks wrote:
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.
This would be a fine point to highlight in the comments around this
check!
The comment at line 435 did serve to tip me off that something unusual
was going on with this code, so it served its purpose. But I'll admit
that it's not as explicit as it could be. How about this:
// Use object identity comparisons with String constants for
performance
// reasons (these values are used heavily within the JDK).
I recommend explicitly mentioning interning "Using identity comparison
against known-interned strings for performance benefit."
-Joe
