Request for review : 7121314 : Behavior mismatch between AbstractCollection.toArray(T[] ) and its spec
David Holmes
david.holmes at oracle.com
Sun Apr 1 13:14:11 UTC 2012
Thanks Sean. I'd say this is done.
David
On 1/04/2012 7:24 PM, Sean Chou wrote:
> Hi David,
>
> I made the new webrev with your modified testcase:
> http://cr.openjdk.java.net/~zhouyx/7121314/webrev.04/ .
>
>
> The link to the bug:
> http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7121314
> The link to the start of the thread:
> http://mail.openjdk.java.net/pipermail/core-libs-dev/2012-March/009512.html
>
>
> On Sun, Apr 1, 2012 at 11:25 AM, David Holmes <david.holmes at oracle.com
> <mailto:david.holmes at oracle.com>> wrote:
>
> Simplified testcase below.
>
> Let's finalise this please.
>
> Thanks,
> David
>
>
> On 1/04/2012 1:08 PM, Sean Chou wrote:
>
> Hi Ulf,
>
> This is a regression testcase, there is no performance issue or
> future refactoring.
> Please wait for David's comments.
>
> On Sun, Apr 1, 2012 at 7:22 AM, Ulf Zibis <Ulf.Zibis at gmx.de
> <mailto:Ulf.Zibis at gmx.de>
> <mailto:Ulf.Zibis at gmx.de <mailto:Ulf.Zibis at gmx.de>>> wrote:
>
> Hi Sean,
>
> thanks for your effort.
>
> Am 31.03.2012 11:43, schrieb Sean Chou:
>
> Hi David and Ulf,
>
> The new webrev is at:
> http://cr.openjdk.java.net/~____zhouyx/7121314/webrev.03/
> <http://cr.openjdk.java.net/~__zhouyx/7121314/webrev.03/>
> <http://cr.openjdk.java.net/~__zhouyx/7121314/webrev.03/
> <http://cr.openjdk.java.net/~zhouyx/7121314/webrev.03/>>
> <http://cr.openjdk.java.net/%____7Ezhouyx/7121314/webrev.03/
> <http://cr.openjdk.java.net/%__7Ezhouyx/7121314/webrev.03/>
>
> <http://cr.openjdk.java.net/%__7Ezhouyx/7121314/webrev.03/
> <http://cr.openjdk.java.net/%7Ezhouyx/7121314/webrev.03/>>> .
>
>
> About the fix, I remained the previous one.
> About the testcase, I merged the 3 files into one.
> During merging, there are 2 modifications:
> 1. I added static modifier to the 3 classes, which are
> enclosed
> by class ToArrayTest;
>
> You do not need the indirection via main()...run()...test()
> if you
> have all in 1 file. This was only necessary to feature a general
> usability of InfraStructure. You can go back to David's 1 +
> 1 nested
> class approach replacing TConcurrentHashMapX by
> TestCollection and
> maybe rename realMain() to test().
> Additionally, loading 4 classes for 1 test would have some
> performance impact on the test run, which could be avoided.
>
>
> 2. I removed field TestCollection.fixedSize, which is
> never read
> after Ulf fixed the bug in testcase.
>
> This field would serve to "reset" the TestCollection to fixed
> default size without the need of new instantiation for later
> refactoring or testcase addition.
>
> As just discussed before, the doc for setSizeSequence() could be
> little more specific:
> 71 /*
> 72 * Sets the values that size() will return on
> each use.
> The first
> 73 * call to size will return sizes[0], then sizes[1]
> etc. This
> 74 * allows us to emulate a concurrent change to the
> contents of
> 75 * the collection without having to perform
> concurrent
> changes.
> 76 * If sizes[n+1] contains a larger value than
> on last
> n-th invocation,
> 77 * the collection will appear to have shrunk when
> iterated; if a
> 78 * smaller value then the collection will appear to
> have grown.
> 79 * When the last element of sizes is reached, the
> collection will
> 80 * appear size-fixed.
> 81 */
>
>
> The link to the bug:
> http://bugs.sun.com/____bugdatabase/view_bug.do?bug_____id=7121314
> <http://bugs.sun.com/__bugdatabase/view_bug.do?bug___id=7121314>
>
> <http://bugs.sun.com/__bugdatabase/view_bug.do?bug___id=7121314
> <http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7121314>>
> The link to the start of the thread:
> http://mail.openjdk.java.net/____pipermail/core-libs-dev/2012-____March/009512.html
> <http://mail.openjdk.java.net/__pipermail/core-libs-dev/2012-__March/009512.html>
>
> <http://mail.openjdk.java.net/__pipermail/core-libs-dev/2012-__March/009512.html
> <http://mail.openjdk.java.net/pipermail/core-libs-dev/2012-March/009512.html>>
>
> Good idea, to repeat these links.
>
> -Ulf
>
>
>
>
> --
> Best Regards,
> Sean Chou
>
>
>
> /*
> * Copyright (c) 2012, Oracle and/or its affiliates. All rights
> reserved.
> * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
> *
> * This code is free software; you can redistribute it and/or modify it
> * under the terms of the GNU General Public License version 2 only, as
> * published by the Free Software Foundation.
> *
> * This code is distributed in the hope that it will be useful, but
> WITHOUT
> * ANY WARRANTY; without even the implied warranty of
> MERCHANTABILITY or
> * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public
> License
> * version 2 for more details (a copy is included in the LICENSE
> file that
> * accompanied this code).
> *
> * You should have received a copy of the GNU General Public
> License version
> * 2 along with this work; if not, write to the Free Software
> Foundation,
> * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
> *
> * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA
> 94065 USA
> * or visit www.oracle.com <http://www.oracle.com> if you need
> additional information or have any
> * questions.
> */
>
> /*
> * @test
> * @bug 7121314
> * @summary AbstractCollection.toArray(T[]__) doesn't return the
> given array
> * in concurrent modification.
> * @author Ulf Zibis, David Holmes
> */
>
> import java.util.AbstractCollection;
> import java.util.Arrays;
> import java.util.Iterator;
>
> public class ToArrayTest {
>
> static class TestCollection<E> extends AbstractCollection<E> {
>
> private final E[] elements;
>
>
> private int[] sizes;
> private int nextSize;
>
> public TestCollection(E[] elements) {
> this.elements = elements;
> setSizeSequence(new int[]{elements.length});
> }
>
> /*
> * Sets the values that size() will return on each use. The next
>
> * call to size will return sizes[0], then sizes[1] etc. This
> * allows us to emulate a concurrent change to the contents of
> * the collection without having to perform concurrent changes.
> * If sizes[n+1] contains a larger value, the collection
> will appear to
> * have shrunk when iterated; if a smaller value then the
> * collection will appear to have grown when iterated
>
> */
> void setSizeSequence(int... sizes) {
> this.sizes = sizes;
> nextSize = 0;
> }
>
> /* can change collection's size after each invocation */
>
> @Override
> public int size() {
> return sizes[nextSize == sizes.length-1 ? nextSize :
> nextSize++];
> }
>
> @Override
> public Iterator<E> iterator() {
> return new Iterator<E>() {
>
> 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.");
> }
> };
> }
> }
>
> static final Object[] OBJECTS = { new Object(), new Object(), new
> Object() };
> static final TestCollection<?> CANDIDATE = new
> TestCollection<Object>(__OBJECTS);
> static final int CAP = OBJECTS.length; // capacity of the CANDIDATE
> static final int LAST = CAP - 1; // last possible array index
> Object[] a;
> Object[] res;
>
> int last() { return a.length - 1; }
>
> protected void test() throws Throwable {
> // Check array type conversion
> res = new TestCollection<>(new Object[]{"1", "2"}).toArray(new
> String[0]);
> check(res instanceof String[]);
> check(res.length == 2);
> check(res[1] == "2");
>
> // Check incompatible type of target array
> try {
> res = CANDIDATE.toArray(new String[CAP]);
> check(false);
> } catch (Throwable t) {
> check(t instanceof ArrayStoreException);
> }
>
> // Check more elements than a.length
> a = new Object[CAP-1]; // appears too small
> res = CANDIDATE.toArray(a);
> check(res != a);
> check(res[LAST] != null);
>
> // Check equal elements as a.length
> a = new Object[CAP]; // appears to match
> res = CANDIDATE.toArray(a);
> check(res == a);
> check(res[last()] != null);
>
> // Check equal elements as a.length
> a = new Object[CAP+1]; // appears too big
> res = CANDIDATE.toArray(a);
> check(res == a);
> check(res[last()] == null);
>
> // Check less elements than expected, but more than a.length
> a = new Object[CAP-2]; // appears too small
> CANDIDATE.setSizeSequence(CAP, CAP-1);
> res = CANDIDATE.toArray(a);
> check(res != a);
> check(res.length == CAP-1);
> check(res[LAST-1] != null);
>
> // Check less elements than expected, but equal as a.length
> a = Arrays.copyOf(OBJECTS, CAP); // appears to match
> CANDIDATE.setSizeSequence(CAP, CAP-1);
> res = CANDIDATE.toArray(a);
> check(res == a);
> check(res[last()] == null);
>
> // Check more elements than expected and more than a.length
> a = new Object[CAP-1]; // appears to match
> CANDIDATE.setSizeSequence(CAP-__1, CAP);
> res = CANDIDATE.toArray(a);
> check(res != a);
> check(res[LAST] != null);
>
> // Check more elements than expected, but equal as a.length
> a = new Object[CAP-1]; // appears to match
> CANDIDATE.setSizeSequence(CAP-__2, CAP-1);
> res = CANDIDATE.toArray(a);
> check(res == a);
> check(res[last()] != null);
>
> // Check more elements than expected, but less than a.length
> a = Arrays.copyOf(OBJECTS, CAP); // appears to match
> CANDIDATE.setSizeSequence(CAP-__2, CAP-1);
> res = CANDIDATE.toArray(a);
> check(res == a);
> check(res[last()] == null);
>
> test_7121314();
> }
>
> /*
> * Major target of this testcase, bug 7121314.
> */
> protected void test_7121314() throws Throwable {
> // Check equal elements as a.length, but less than expected
> a = new Object[CAP-1]; // appears too small
> CANDIDATE.setSizeSequence(CAP, CAP-1);
> res = CANDIDATE.toArray(a);
> check(res == a);
> check(res[last()] != null);
>
> // Check less elements than a.length and less than expected
> a = Arrays.copyOf(OBJECTS, CAP-1); // appears too small
> CANDIDATE.setSizeSequence(CAP, CAP-2);
> res = CANDIDATE.toArray(a);
> check(res == a);
> check(res[last()] == null);
>
> }
>
>
> public static void main(String[] args) throws Throwable {
> ToArrayTest testcase = new ToArrayTest();
> try { testcase.test(); } catch (Throwable t) { unexpected(t); }
>
> System.out.printf("%nPassed = %d, failed = %d%n%n", passed, failed);
> if (failed > 0) throw new Exception("Some tests failed");
> }
>
> private static volatile int passed = 0, failed = 0;
> private static void pass() { passed++; }
> private static void fail() { failed++; Thread.dumpStack(); }
> private static void fail(String msg) { System.out.println(msg);
> fail(); }
> private static void unexpected(Throwable t) { failed++;
> t.printStackTrace(); }
>
> static void check(boolean cond) { if (cond) pass(); else fail(); }
> static void equals(Object x, Object y) {
>
> if (x == null ? y == null : x.equals(y)) pass();
> else {System.out.println(x + " not equal to " + y); fail(); }}
>
> }
>
>
>
>
>
> --
> Best Regards,
> Sean Chou
>
More information about the core-libs-dev
mailing list