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

[Joe Darcy]

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