What happened with the files? It seems the patch that was uploaded got all messed up. Git5 mis-identified renames, perhaps?
2010/3/31 <[email protected]> > > 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.
