Thank you Tagir for the review!

On 29.01.2016 7:16, Tagir F. Valeev wrote:
Hello!

AbstractList::subListRangeCheck could be replaced with new
Objects::checkFromToIndex for consistency with other methods. Also
Yes, that would be good.
However, for AbstractList.subList() we have this in spec:
* @throws IllegalArgumentException if the endpoint indices are out of order
     *         {@code (fromIndex > toIndex)}

but the 2-args checkFromToIndex() will throw IndexOutOfBoundsException instead.

If we use 3-args checkFromToIndex, it wouldn't save us anything in the terms of lines of code.

It's misfortune that the spec of subList() doesn't match the spec of similar methods, like CharSequence.subSequence() or String.substring(). It even contradicts the spec of List.subList(), which declares only IOOB to be thrown!


AbstractList::rangeCheck could be replaced with Obejcts::checkIndex.
The same for ArrayList class.

Right.
Here's the updated webrev:
http://cr.openjdk.java.net/~igerasim/8079136/06/webrev/

Sincerely yours,
Ivan

Otherwise looks good to me.

With best regards,
Tagir Valeev.

IG> Hello everyone!

IG> I'd like to respin the discussion.
IG> The previous attempt can be found here:
IG> http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-May/033159.html

IG> BUGURL: https://bugs.openjdk.java.net/browse/JDK-8079136
IG> WEBREV: http://cr.openjdk.java.net/~igerasim/8079136/05/webrev/

IG> Here's the summary of the proposed changes:
IG> 1)
IG> Sublist of an AbstractList (AbstractList.SubList class) now maintains a
IG> link to the root AbstractList, and not only to the immediate parent list.
IG> This allows to perform modifications on the chain of sub-lists in a loop
IG> instead of using recursion (which, in particular, helps avoid the
IG> stack-overflow problem).
IG> The sublist is still backed by its parent list, in sense that all the
IG> modifications are correctly reflected in the backing list (as well as in
IG> all the grand parents the sublist, if any), so the fix does not violate
IG> the existing specification.

IG> 2)
IG> It is proposed to update the spec of AbstractList.subList() in the
IG> @implSpec section by removing the words about private fields.
IG> With the fix, some of those private fields are removed.

IG> 3)
IG> A similar change is proposed for the ArrayList.SubList class.

IG> 4)
IG> Two regression tests are added:
IG> NestedSubList.java - demonstrates a stack-overflow problem when dealing
IG> with a long chain of sublists,
IG> SubList.java - tests basic functionality of sub-lists, which helps us
IG> make sure nothing is broken with the proposed change.

IG> If people agree that the fix is good, I'll file a CCC request for
IG> changing the spec of AbstractList.subList().

IG> Sincerely yours,
IG> Ivan



Reply via email to