The biggie is MutableArray.java:60....
http://gwt-code-reviews.appspot.com/291801/diff/4001/5003 File bikeshed/src/com/google/gwt/collections/ImmutableArrayEmptyImpl.java (right): http://gwt-code-reviews.appspot.com/291801/diff/4001/5003#newcode27 bikeshed/src/com/google/gwt/collections/ImmutableArrayEmptyImpl.java:27: Assertions.assertIndexInRange(index, 0, 0); this will always fail... is it worth some more specific assertion? Or at least a syntax sugar that reduces to the same, but is clear at this call site that 0 is actually excluded? http://gwt-code-reviews.appspot.com/291801/diff/4001/5004 File bikeshed/src/com/google/gwt/collections/ImmutableArrayImpl.java (right): http://gwt-code-reviews.appspot.com/291801/diff/4001/5004#newcode25 bikeshed/src/com/google/gwt/collections/ImmutableArrayImpl.java:25: private final E[] elems; you might consider making this default visibility, to test that we don't copy, since that's the point to some of the hoops we're doing (see comment in ImmutableArrayTest). http://gwt-code-reviews.appspot.com/291801/diff/4001/5004#newcode28 bikeshed/src/com/google/gwt/collections/ImmutableArrayImpl.java:28: Assertions.assertNotNull(elems); do you also want to assert that elem.length > 0? http://gwt-code-reviews.appspot.com/291801/diff/4001/5005 File bikeshed/src/com/google/gwt/collections/MutableArray.java (right): http://gwt-code-reviews.appspot.com/291801/diff/4001/5005#newcode60 bikeshed/src/com/google/gwt/collections/MutableArray.java:60: elems = null; Huh?! Can't I keep using my existing MutableArray, post-freeze, so long as I don't change it?! Doesn't this line zero it out (but leave it frozen)? http://gwt-code-reviews.appspot.com/291801/diff/4001/5006 File bikeshed/test/com/google/gwt/collections/ImmutableArrayTest.java (right): http://gwt-code-reviews.appspot.com/291801/diff/4001/5006#newcode58 bikeshed/test/com/google/gwt/collections/ImmutableArrayTest.java:58: I think that if you test ma.size() here, you get an unexpected 0 rather than 2... http://gwt-code-reviews.appspot.com/291801/diff/4001/5006#newcode60 bikeshed/test/com/google/gwt/collections/ImmutableArrayTest.java:60: ma.add(1); I like 0, 1, many testing in general, but I'm not sure the 1 case helps here. http://gwt-code-reviews.appspot.com/291801/diff/4001/5006#newcode158 bikeshed/test/com/google/gwt/collections/ImmutableArrayTest.java:158: Something I'd like to test, but we don't have visibility to here, is that we didn't copy... i.e. that ia1.elems === ia2.elems === ma.elems. http://gwt-code-reviews.appspot.com/291801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors To unsubscribe from this group, send email to google-web-toolkit-contributors+unsubscribegooglegroups.com or reply to this email with the words "REMOVE ME" as the subject.
