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

[Sean Chou]

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<E> extends AbstractCollection<E> {
>>
>>       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<E> 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