The changes look good to me.
I am starting to come into agreement with Remi though that unless a type
specific array can be created for situations like the E[] elements array in
ArrayDeque then it should be declared as Object[] array since that's what is
actually created.
Mike
On Aug 31 2012, at 12:06 , Dan Xu wrote:
> On 08/30/2012 06:45 PM, Stuart Marks wrote:
>> On 8/30/12 7:14 AM, Ulf Zibis wrote:
>>> Am 30.08.2012 01:20, schrieb Stuart Marks:
>>>> On 8/29/12 4:36 AM, Ulf Zibis wrote:
>>>>>> @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.
>>>
>>> I can see that it is reasonable/possible to bind a
>>> @SuppressWarnings("unchecked") to a declaration which contains the unchecked
>>> assignment, but which declaration should contain a fallthrough case?
>>>
>>> So shouldn't @SuppressWarnings("fallthrough") better be binded to a switch
>>> or
>>> case statement?
>>
>> In principle it would be nice to place the @SuppressWarnings right next to
>> the switch statement for which the warnings are suppressed. However, it is a
>> syntactic restriction of the Java programming language that annotations can
>> only be used as modifiers on declarations, specifically package, class,
>> method, method parameter, constructor, and local variable declarations. See
>> JLS 9.7 [1]. It is not legal to place annotations on statements. So, the
>> SuppressWarnings annotation has to be placed on a declaration that encloses
>> the switch statement in question. In this case that's the declaration of the
>> entire method.
>>
>> [1] http://docs.oracle.com/javase/specs/jls/se7/html/jls-9.html#jls-9.7
>>
>> It turns out that this discussion is moot, as the switch fall-through is
>> actually unnecessary! The comma case falls through to the next case, but all
>> that does is break. The warning can be eliminated simply by replacing
>> /*FALLTHROUGH*/ with a break statement. This removes the need for the
>> SuppressWarnings annotation.
>>
>> This permissions-parsing code occurs in several places around the JDK,
>> copied and slightly modified. Some of them use the string identity
>> comparison optimization, and some have this switch fall through. (We've
>> discussed this previously. See [2].) Accordingly, I've asked Dan to adjust a
>> couple of these other cases to have the same comment about string identity
>> comparisons, and to change the switch statement to avoid the fall-through
>> entirely.
>>
>> [2] http://mail.openjdk.java.net/pipermail/jdk8-dev/2011-December/000415.html
>>
>> While I'm reluctant to expand the scope of this changeset, I think it's
>> important to keep these five occurrences consistent. Obviously it would be
>> nicer to unify the five copies into one, but this is a much bigger change
>> that I think is clearly out of scope.
>>
>> Dan will issue another webrev with these changes shortly.
>>
>> s'marks
>>
>
> I have adjusted my fix with the above suggestions.
>
> The latest webrev is posted at,
> http://cr.openjdk.java.net/~dxu/7193406/webrev.05/. Thanks for your comments!
>
> -Dan
