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

[Mike Duigou]

This looks good to me.

Mike

On Mar 12 2012, at 23:58 , Sean Chou wrote:

> Hi Ulf and David,
> 
>    I modified the patch and added the testcase, it's now :
> http://cr.openjdk.java.net/~zhouyx/7121314/webrev.02/    .
> 
>    Ulf's compact version is used, it looks beautiful; however I replaced
> the Math.min part with if statement because if statement is more intuitive
> and I don't think there is any performance concern. But it is not so
> compact now...
>    Also I added the equal size case and @author to testcase.
> 
>    There is a little problem when I created the webrev, I don't know how
> to change the "contributed-by" information for the testcase, so the list is
> still Ulf's and my emails.

It looks like the webrev is being generated from an mq patch. In this case do a 'hg qrefresh -e' before doing the 'hg qfinish' to edit the commit message.

Mike

>    Please take a look again.
> 
> On Fri, Mar 9, 2012 at 8:45 PM, Ulf Zibis <Ulf.Zibis at gmx.de> wrote:
> 
>> Am 09.03.2012 09:16, schrieb Sean Chou:
>> 
>>> Hi all,
>>> 
>>>   AbstractCollection.toArray(T[] ) might return a new array even if the
>>> given array has enough room for the returned elements when it is
>>> concurrently modified. This behavior violates the spec documented in
>>> java.util.Collection .
>>>   This patch checks the size of returned array and copies the elements
>>> to return to the given array if it is large enough.
>>> 
>>>   The webrev is at :
>>> http://cr.openjdk.java.net/~**zhouyx/7121314/webrev.00/<http://cr.openjdk.java.net/~zhouyx/7121314/webrev.00/><
>>> http://cr.openjdk.java.net/%**7Ezhouyx/7121314/webrev.00/<http://cr.openjdk.java.net/%7Ezhouyx/7121314/webrev.00/>
>>>> 
>>> 
>> 
>> More compact and marginally faster:
>> 182             if (!it.hasNext()) { // fewer elements than expected
>> 183                 if (a == r) {
>> 184                     a[i] = null; // null-terminate
>> 185                 } else if (a.length < i) {
>> 186                     return Arrays.copyOf(r, i);
>> 187                 } else {
>> 188                     System.arraycopy(r, 0, a, 0, Math.min(++i,
>> a.length()); // ensure null-termination
>> 189                 }
>> 190                 return a;
>> 191             }
>> 
>> 
>> There is a test case in the previous discussion. It is not included in
>>> the webrev, because the testcase is heavily implementation dependent. I
>>> will add it if it is requested.
>>> 
>> I think, we should have a testcase for all 3 cases: fewer / equal / less
>> elements than expected.
>> Additionally I think, the correct null-termination should be tested.
>> 
>> 
>>         Thread[] threads = new Thread[2];
>>>        threads[0] = new Thread(new ToArrayThread());
>>>        threads[1] = new Thread(new RemoveThread());
>>> 
>> Why so complicated?
>> IMHO better:
>>       Thread toArrayThread = new Thread(new ToArrayThread());
>>       Thread removeThread = new Thread(new RemoveThread());
>> 
>> - Ulf
>> 
>> 
> 
> 
> -- 
> Best Regards,
> Sean Chou