http://gwt-code-reviews.appspot.com/232801/diff/3001/4002 File bikeshed/src/com/google/gwt/collections/Assertions.java (right):
http://gwt-code-reviews.appspot.com/232801/diff/3001/4002#newcode24 bikeshed/src/com/google/gwt/collections/Assertions.java:24: assert (index >= 0 && index < maxExclusive) : msgIndexOutOfRange(index, minInclusive, On 2010/03/22 18:38:53, Dan Rice wrote:
minInclusive is never checked here
Done. http://gwt-code-reviews.appspot.com/232801/diff/3001/4002#newcode37 bikeshed/src/com/google/gwt/collections/Assertions.java:37: static String msgShouldNotBeNull() { On 2010/03/22 15:59:55, fabbott wrote:
I'm not entirely sure these are useful (vice just using the constants
in-line in
the sole call sites), but okay...
Let's inline it and refactor later if there is a need for it. http://gwt-code-reviews.appspot.com/232801/diff/3001/4007 File bikeshed/src/com/google/gwt/collections/MutableArray.java (right): http://gwt-code-reviews.appspot.com/232801/diff/3001/4007#newcode23 bikeshed/src/com/google/gwt/collections/MutableArray.java:23: // Used when there is exactly one element in progress On 2010/03/22 15:59:55, fabbott wrote:
"in progress" doesn't mean much for me here.
Done. Removed "in progress" http://gwt-code-reviews.appspot.com/232801/diff/3001/4007#newcode38 bikeshed/src/com/google/gwt/collections/MutableArray.java:38: @SuppressWarnings("unchecked") On 2010/03/22 15:59:55, fabbott wrote:
what's the unchecked warning from?
Unused. Removed. http://gwt-code-reviews.appspot.com/232801/diff/3001/4007#newcode56 bikeshed/src/com/google/gwt/collections/MutableArray.java:56: } else if (singleElem != null) { On 2010/03/22 15:59:55, fabbott wrote:
two questions: first, is the whole singleElem special case actually
gaining
anything here? second, am I allowed to have an array of size 1
containing only
null? (And if not, why not, and how do I know?)
It seems overly complex. I removed the single element case. http://gwt-code-reviews.appspot.com/232801/diff/3001/4007#newcode75 bikeshed/src/com/google/gwt/collections/MutableArray.java:75: On 2010/03/22 15:59:55, fabbott wrote:
I think you need a check here (and remove and set) against isfrozen?
Yes, I will add the isFrozen checks at the same time I add the immutable functionality. It is just that I wanted to begin by reviewing the Mutable and Read-only cases. http://gwt-code-reviews.appspot.com/232801/diff/3001/4007#newcode79 bikeshed/src/com/google/gwt/collections/MutableArray.java:79: // TODO(bruce): grow smartly On 2010/03/22 15:59:55, fabbott wrote:
not likely to be bruce. (Not sure we attribute our todo's generally,
anyway.) Fixed and made more explicit. http://gwt-code-reviews.appspot.com/232801/diff/3001/4007#newcode92 bikeshed/src/com/google/gwt/collections/MutableArray.java:92: singleElem = elem; On 2010/03/22 15:59:55, fabbott wrote:
you lose if elem==null
Removed the singleElem contruct. I will revisit the idea when we get to benchmarking and optimization. http://gwt-code-reviews.appspot.com/232801/diff/3001/4007#newcode110 bikeshed/src/com/google/gwt/collections/MutableArray.java:110: // Bad! But only happens if index is bad and assertions are off On 2010/03/22 15:59:55, fabbott wrote:
I know bruce was imagining minimal safety overhead, but I'd like to
see an
assertion here, just in case we're someday wrong/buggy about the
precondition
assertions. I can live with assertions-only correctness checking, but
get
queasy about deliberate holes.
Added assert(false) to force an assert if this condition is ever hit. http://gwt-code-reviews.appspot.com/232801/diff/3001/4007#newcode131 bikeshed/src/com/google/gwt/collections/MutableArray.java:131: // Bad! But only happens if index is bad and assertions are off On 2010/03/22 15:59:55, fabbott wrote:
as above, wrt assertions for checking.
Done. http://gwt-code-reviews.appspot.com/232801/diff/3001/4008 File bikeshed/test/com/google/gwt/collections/ObjectArrayTest.java (right): http://gwt-code-reviews.appspot.com/232801/diff/3001/4008#newcode29 bikeshed/test/com/google/gwt/collections/ObjectArrayTest.java:29: throw new IllegalStateException("This test case requires assertions to be enabled"); On 2010/03/22 15:59:55, fabbott wrote:
um. I dunno that this should be true, or if true I think we might
want it to
pass (but be a no-op) rather than fail when run without 'em.
Added guards to disable tests that depend on assertion when assertion is disabled. http://gwt-code-reviews.appspot.com/232801/diff/3001/4008#newcode108 bikeshed/test/com/google/gwt/collections/ObjectArrayTest.java:108: b.add(null); On 2010/03/22 15:59:55, fabbott wrote:
yeah, try doing this (add of null) as your first item. ;-)
Done. http://gwt-code-reviews.appspot.com/232801/diff/3001/4008#newcode126 bikeshed/test/com/google/gwt/collections/ObjectArrayTest.java:126: b.set(3, "eclair"); On 2010/03/22 15:59:55, fabbott wrote:
check result?
Done. http://gwt-code-reviews.appspot.com/232801/diff/3001/4008#newcode177 bikeshed/test/com/google/gwt/collections/ObjectArrayTest.java:177: fail("That should have been okay"); On 2010/03/22 15:59:55, fabbott wrote:
can you check the assertion message, here and elsewhere, to be sure
it's the one
we want? I remember seeing some issues with junit 4 where assertions
could
cause problems (e.g. the junit assertions and your SUT assertions
co-mingled).
Also, again, I'd rather you checked assertion state and decided what
to expect
(or short-circuited the test) then, so the test will "work" in either
mode. I have removed the try/catch wrapping. http://gwt-code-reviews.appspot.com/232801/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.
