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

Sean Chou zhouyx at linux.vnet.ibm.com
Mon Mar 26 05:02:16 UTC 2012

Hi Ulf,

Comment inlined.

On Sat, Mar 24, 2012 at 5:09 AM, Ulf Zibis <Ulf.Zibis at gmx.de> wrote:

Hi Sean,

Am 23.03.2012 07:45, schrieb Sean Chou:

Hi Ulf,

I'm sorry I didn't quite get your testcase. This is not a testcase, just a draft of an additionally more flexible test object to replace TConcurrentHashMap/2. Will you please provide a jtreg style testcase with main method ? Well, as I'm missing your agreement, that David's test implementation doesn't guarantee to test the right toArray method of AbstractCollection as I explained before, I'm afraid that additional effort would be for garbage. Every testcase or fix goes this way, like the first testcase I provided. If your suggestion is valuable, I don't think it will be wasted.

Aside, as the instantiation of (several) ConcurrentHashMap subclassed test objects seems more expensive, I believe, my simple TestCollection would increase the performance of the testcases.

What's the exact problem you want to fix in this case?

I was thinking you were worried by the comment "// inherits from AbstractCollection.toArray()" and "map2", if that was the case, I would like to modify. This is additionally true, but not my main issue, as later to me it turned out, that "// inherits from AbstractCollection.toArray()" is not enough to guarantee that the right toArray method is actually tested, aside to look not obvious IMO. If you really have this concern, you should provide your testcase !

-Ulf P.S.: better rename to: void setPseudoConcurrentSizeCourse(**int... sizes) {...}

On Fri, Mar 23, 2012 at 5:57 AM, Ulf Zibis <Ulf.Zibis at gmx.de <mailto:_ _Ulf.Zibis at gmx.de>> wrote: Hi Sean, bad news ;-) ... Am 22.03.2012 08:28, schrieb Sean Chou: Hi Ulf, I'm glad you agreed my suggestion. To all: Can this patch be committed as it has been reviewed by David Holmes and Mike Duigou, and Ulf also says agreed ?

I agree with your implementation of AbstractCollection, but NOT with the test. For correct testing I suggest to use: /** * * @author Ulf Zibis */ public class TestCollection extends AbstractCollection { private E[] elements; private int[] sizes; private int nextSize; public TestCollection(E[] elements) { this.elements = elements; setConcurrentSizeCourse(null); } void setConcurrentSizeCourse(int... sizes) { this.sizes = sizes == null ? new int[]{elements.length} : sizes; nextSize = 0; } @Override public int size() { return sizes[nextSize == sizes.length-1 ? nextSize : nextSize++]; } @Override public Iterator iterator() { return new Iterator<>() { int pos = 0; public boolean hasNext() { return pos < sizes[nextSize]; } public E next() { return elements[pos++]; } public void remove() { throw new UnsupportedOperationException(**"Not supported yet."); } }; } } -Ulf

-- Best Regards, Sean Chou

-- Best Regards, Sean Chou

More information about the core-libs-dev mailing list