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

Mike Duigou mike.duigou at oracle.com
Tue Mar 13 17:26:03 UTC 2012

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

More information about the core-libs-dev mailing list