On 02/04/2013 05:44, Mike Duigou wrote:
Hello all;
Last night while pushing another changeset I accidentally pushed a changeset
for JKD-7143928. Since the review and testing was not complete on this issue I
have backed out that changeset and created a new bug number to continue
development. The new webrev to complete the review is:
http://cr.openjdk.java.net/~mduigou/JDK-8011200/0/webrev/
It is currently unchanged from the last posted changeset for 7143928.
Mike
I skimmed through the webrev as lazily creating the backing arrays will
be help in some environments.
For ArrayList then it might be good to add a constant
DEFAULT_INITIAL_CAPACITY or some such to avoid repeating the specified
default.
I agree with Martin's comment that ArrayList.clear will now clear
elementData.length elements rather than size so I assume you'll address
that.
I also agree with Martin on keeping the coding style consistent,
particularly the missing space in "if(", and missing braces in places
such as HashMap.writeObject.
For ArrayList.readObject then you read the array length in as
"initialCapacity" which I think is a bit confusing given the current
semantics. An alternative is to just not change readObject unless there
is evidence that it is common to reconstitute empty ArrayLists.
For HashMap.indexFor then I assume this isn't an assert because it is
used early in the VM startup. This should probably be changed to
InternalError and the message needs to be changed too.
Do you envisage usage of inflateTable in LinkedHashMap? I'm wondering
why it isn't private.
HashMap.writeObject - the update to the @serialData text will change the
wording emitting in the javadoc. I mention this as I think you are
planning to push this to jdk7u at some point.
I don't have time to go through the test in detail at this time but it
looks like it has the original bug number and I assume that will need to
change now.
-Alan.
