Comments as noted; also, what semantics do we want for equality? (Identity is certainly cheapest, and probably good, just wanted to make an explicit choice there).
I think you're buggy if your first element is null, and I'm not entirely sure the singleElem special case is worth the time-cost of your decision branches. I think you will be buggy, once you add immutability, if you don't check isfrozen. 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#newcode37 bikeshed/src/com/google/gwt/collections/Assertions.java:37: static String msgShouldNotBeNull() { I'm not entirely sure these are useful (vice just using the constants in-line in the sole call sites), but okay... 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 "in progress" doesn't mean much for me here. http://gwt-code-reviews.appspot.com/232801/diff/3001/4007#newcode38 bikeshed/src/com/google/gwt/collections/MutableArray.java:38: @SuppressWarnings("unchecked") what's the unchecked warning from? http://gwt-code-reviews.appspot.com/232801/diff/3001/4007#newcode56 bikeshed/src/com/google/gwt/collections/MutableArray.java:56: } else if (singleElem != null) { 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?) http://gwt-code-reviews.appspot.com/232801/diff/3001/4007#newcode75 bikeshed/src/com/google/gwt/collections/MutableArray.java:75: I think you need a check here (and remove and set) against isfrozen? (Or, if immutability is a copy op, you don't need isfrozen, one or the other...) http://gwt-code-reviews.appspot.com/232801/diff/3001/4007#newcode79 bikeshed/src/com/google/gwt/collections/MutableArray.java:79: // TODO(bruce): grow smartly not likely to be bruce. (Not sure we attribute our todo's generally, anyway.) http://gwt-code-reviews.appspot.com/232801/diff/3001/4007#newcode92 bikeshed/src/com/google/gwt/collections/MutableArray.java:92: singleElem = elem; you lose if elem==null 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 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. 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 as above, wrt assertions for checking. 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"); 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. http://gwt-code-reviews.appspot.com/232801/diff/3001/4008#newcode108 bikeshed/test/com/google/gwt/collections/ObjectArrayTest.java:108: b.add(null); yeah, try doing this (add of null) as your first item. ;-) http://gwt-code-reviews.appspot.com/232801/diff/3001/4008#newcode126 bikeshed/test/com/google/gwt/collections/ObjectArrayTest.java:126: b.set(3, "eclair"); check result? 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"); 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. 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.
