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

[Sean Chou]

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.

    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