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); On 2010/03/30 20:25:29, fabbott wrote:
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?
Done. 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; On 2010/03/30 20:25:29, fabbott wrote:
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).
Done. http://gwt-code-reviews.appspot.com/291801/diff/4001/5004#newcode28 bikeshed/src/com/google/gwt/collections/ImmutableArrayImpl.java:28: Assertions.assertNotNull(elems); On 2010/03/30 20:25:29, fabbott wrote:
do you also want to assert that elem.length > 0?
Not really. length == 0 is OK. It is just that we chose to use ImmutableArrayEmptyImpl for our implementation. Should I also add test cases for the ImmutableX classes? 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; On 2010/03/30 20:25:29, fabbott wrote:
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)? Ah, yes. I guess this was too extreme. The intention was to discourage using MutableArray after freezing. Using ImmutableArray was the way to access in that case. But now that you mention it I can see situations where that may be cumbersome. Removed clearing the backing array. 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: On 2010/03/30 20:25:29, fabbott wrote:
I think that if you test ma.size() here, you get an unexpected 0
rather than
2...
Done. http://gwt-code-reviews.appspot.com/291801/diff/4001/5006#newcode60 bikeshed/test/com/google/gwt/collections/ImmutableArrayTest.java:60: ma.add(1); On 2010/03/30 20:25:29, fabbott wrote:
I like 0, 1, many testing in general, but I'm not sure the 1 case
helps here. Done. http://gwt-code-reviews.appspot.com/291801/diff/4001/5006#newcode158 bikeshed/test/com/google/gwt/collections/ImmutableArrayTest.java:158: On 2010/03/30 20:25:29, fabbott wrote:
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.
Done. http://gwt-code-reviews.appspot.com/291801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors To unsubscribe, reply using "remove me" as the subject.
