Start on Guava issue 196: control builder capacity

[Louis Wasserman, 2012-04-27 01:41:58]

I'm not exposing the methods until we all agree on the API, but these would be
useful internally for the rewrite of the immutable collection implementations
that I'm working on with gak.

[Chris Povirk, 2012-04-30 18:47:04]

http://codereview.appspot.com/6092044/diff/1/guava-tests/test/com/google/comm...
File guava-tests/test/com/google/common/collect/ImmutableListTest.java (right):

http://codereview.appspot.com/6092044/diff/1/guava-tests/test/com/google/comm...
guava-tests/test/com/google/common/collect/ImmutableListTest.java:396:
assertEquals(Integer.MAX_VALUE, ImmutableList.expandedCapacity(0,
Integer.MAX_VALUE));
How about the other end?  Do inputs of 0 and 1 work?  (They might not need to
now but maybe in the future?)

http://codereview.appspot.com/6092044/diff/1/guava/src/com/google/common/coll...
File guava/src/com/google/common/collect/ImmutableList.java (right):

http://codereview.appspot.com/6092044/diff/1/guava/src/com/google/common/coll...
guava/src/com/google/common/collect/ImmutableList.java:708: contents[size++] =
checkNotNull(e);
Some quick testing suggests to me that |size| will be incremented even if
checkNotNull fails.  We should have a test for this (and a fix if I'm right).

Likewise for add(E...).

http://codereview.appspot.com/6092044/diff/1/guava/src/com/google/common/coll...
guava/src/com/google/common/collect/ImmutableList.java:764: return new
RegularImmutableList<E>(Arrays.copyOf(contents, size));
Arrays.copyOf won't work under GWT:
https://developers.google.com/web-toolkit/doc/latest/RefJreEmulation#Package_...

[Louis Wasserman, 2012-05-02 17:37:38]

http://codereview.appspot.com/6092044/diff/1/guava-tests/test/com/google/comm...
File guava-tests/test/com/google/common/collect/ImmutableListTest.java (right):

http://codereview.appspot.com/6092044/diff/1/guava-tests/test/com/google/comm...
guava-tests/test/com/google/common/collect/ImmutableListTest.java:396:
assertEquals(Integer.MAX_VALUE, ImmutableList.expandedCapacity(0,
Integer.MAX_VALUE));
On 2012/04/30 18:47:04, Chris Povirk wrote:
> How about the other end?  Do inputs of 0 and 1 work?  (They might not need to
> now but maybe in the future?)

Done.

http://codereview.appspot.com/6092044/diff/1/guava/src/com/google/common/coll...
File guava/src/com/google/common/collect/ImmutableList.java (right):

http://codereview.appspot.com/6092044/diff/1/guava/src/com/google/common/coll...
guava/src/com/google/common/collect/ImmutableList.java:708: contents[size++] =
checkNotNull(e);
On 2012/04/30 18:47:04, Chris Povirk wrote:
> Some quick testing suggests to me that |size| will be incremented even if
> checkNotNull fails.  We should have a test for this (and a fix if I'm right).
> 
> Likewise for add(E...).

Test added...but do we really care about atomic failure here?

http://codereview.appspot.com/6092044/diff/1/guava/src/com/google/common/coll...
guava/src/com/google/common/collect/ImmutableList.java:764: return new
RegularImmutableList<E>(Arrays.copyOf(contents, size));
On 2012/04/30 18:47:04, Chris Povirk wrote:
> Arrays.copyOf won't work under GWT:
>
https://developers.google.com/web-toolkit/doc/latest/RefJreEmulation#Package_...

My research suggests that GWT has the tools to emulate it -- but I'll make those
changes to Platform in a separate CL, and do a more "traditional" copy here for
the moment.

[Louis Wasserman, 2012-05-02 17:51:29]

Update: we were already emulating Arrays.copyOf internally, so I just used that
instead.

[Chris Povirk, 2012-05-02 18:36:21]

LGTM

http://codereview.appspot.com/6092044/diff/1/guava/src/com/google/common/coll...
File guava/src/com/google/common/collect/ImmutableList.java (right):

http://codereview.appspot.com/6092044/diff/1/guava/src/com/google/common/coll...
guava/src/com/google/common/collect/ImmutableList.java:708: contents[size++] =
checkNotNull(e);
On 2012/05/02 17:37:39, Louis Wasserman wrote:
> On 2012/04/30 18:47:04, Chris Povirk wrote:
> > Some quick testing suggests to me that |size| will be incremented even if
> > checkNotNull fails.  We should have a test for this (and a fix if I'm
right).
> > 
> > Likewise for add(E...).
> 
> Test added...but do we really care about atomic failure here?

As with the Multimap(?) thing a while back, I don't see this as about "being
atomic" so much as about "not rendering the object inoperable."  To me, "atomic"
means "all elements go in or none do," which I don't think we need to worry
about.  But if future add() calls are broken, that feels like a worse failure.

http://codereview.appspot.com/6092044/diff/11001/guava-tests/test/com/google/...
File guava-tests/test/com/google/common/collect/ImmutableListTest.java (right):

http://codereview.appspot.com/6092044/diff/11001/guava-tests/test/com/google/...
guava-tests/test/com/google/common/collect/ImmutableListTest.java:414: 
Remove the blank line here and at 428.

[Louis Wasserman, 2012-05-02 18:45:33]

The CL is at
http://code.google.com/r/wassermanlouis-guava/source/detail?r=7686e9806c240a6...
.