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

[Ulf Zibis]

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 .
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