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