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

[David Holmes]

Still looks okay to me.

David

On 13/03/2012 4:58 PM, 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.
>
>      Please take a look again.
>
> On Fri, Mar 9, 2012 at 8:45 PM, Ulf Zibis <Ulf.Zibis at gmx.de
> <mailto: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
>