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