Request for review : 7121314 : Behavior mismatch between AbstractCollection.toArray(T[] ) and its spec

[Sean Chou]

Hi Ulf,
    I'm glad you agreed my suggestion.

To all:
    Can this patch be committed as it has been reviewed by David Holmes and
Mike Duigou, and Ulf also says agreed  ?

On Thu, Mar 22, 2012 at 3:05 AM, Ulf Zibis <Ulf.Zibis at gmx.de> wrote:

> Am 19.03.2012 05:53, schrieb Sean Chou:
>
>> Hi Ulf,
>>    I hope following comments can help reduce your concern.
>>
>>
>> I don't think this footprint is a problem until it is proved to be one.
>> If it is, a better javac can be used to save this footprint, and it would
>> save much more. The actual problem might be reached by
>> http://openjdk.java.net/jeps/**149 <http://openjdk.java.net/jeps/149> .
>>
> As I have no setup here now, I cna't proove it, sorry, but: Many a mickle
> makes a muckle.
>
>
>
>>        With the if statement, it reads "put a null after the elements if
>> there are more space",
>>        while with your code, it reads "copy all the elements from r or
>> copy all elements and 1
>>        more from r if there are more space" and we have to think "what's
>> the next element in r ?
>>        ". In fact, we need look back to find how r is defined "T[] r =
>> a.length >= size ? a :
>>        (T[])java.lang.reflect.Array.**newInstance(a.getClass().**getComponentType(),
>> size);" and go
>>        through the code once more to realize there is a null at that
>> position.
>>
>>    Yes, a better comment would be necessary.
>>
>> A comment just help understand the code, does not remove this thinking.
>>
> Well, but only if one doesn't believe it ;-)
>
>
>  However, 7153238 is a RFE and this is a bug. I would like the bug to be
>> fixed in a way easy to understand if possible. And you can put all the
>> enhancement in the RFE which in fact would do much more then this piece of
>> byte code footprint saving. It is better to do one thing in one bug/RFE.
>>
> Agreed!
>
>
>     But you could reuse it (like Object[] res) with:
>>    Map<String,Object> map = new TConcurrentHashMap<>(); // better: TCHM1
>>    ...
>>    map = new TConcurrentHashMap2<>();
>>    I agree, a little nit, but I had the other "missing" test cases in
>> mind, where the numbering
>>    then could become confusing.
>>
>> The comment"// Check less elements" and " // Check equal elements"
>> clearly describe the two blocks of code are testing different scenarios.
>> And a new definition would emphasize it is a different variable, it is
>> used to test different scenarios.
>>
> With same justification you could have: a1, a2, res1, res2 instead reusing
> a, res.
>
>
>     I would not like to add "// inherits from
>> AbstractCollection.toArray()" , it is obvious and
>>    listed in java doc.
>>
>>    In ConcurrentHashMap.values:
>>       Overrides: values in class AbstractMap<K,V>
>>    In AbstractMap.values:
>>       This implementation returns a collection that subclasses
>> AbstractCollection.
>>    But there is no guarantee, that the returned collection overrides or
>> just delegates to
>>    AbstractCollection.toArray().
>>    In worst case, t.j.u.AC.ToArray doesn't test j.u.
>> AbstractCollection.toArray() at all.
>>
>> Do you mean ConcurrentHashMap returns a collection does not inherit
>> AbstractCollection ? Don't worry about that, ConcurrentHashMap is a class
>> of java collection framework, it won't let that happen. AbstractCollection
>> is born to be inherited, especially by collection classes in java
>> collection framework.
>>
> Yes, it is born to be inherited, and one day someone could override
> AbstractCollection.toArray() in the implementation of the subclassed
> collection returned by AbstractMap.values() without violating the given
> spec. In this case, your test would test the subclassed collection's
> toArray() method, but not AbstractCollection.toArray().
>
> -Ulf
>
>


-- 
Best Regards,
Sean Chou