RFR: 8079136: Accessing a nested sublist leads to StackOverflowError

[Paul Sandoz]

On May 7, 2015, at 1:23 AM, Martin Buchholz <martinrb at google.com> wrote:

> Hi Ivan,
> 
> I'm afraid of these changes - they are hard to review.
> 
> Can't we fix the SOE with a relatively small change to ArrayList.SubList methods that recursively invoke parent methods to use iteration instead,

Since all that would do is eventually modify the underlying list, one might as well go to it directly when one can.

I am ok with this changes as long as we have good tests and coverage.


> i.e. can't you implement updateSizeAndModCount in the existing SubList class?
> 
> ---
> 
> I would make modCount an argument to updateSizeAndModCount.
> 

That would just repeat the same root mod-count accessor for all calls to updateSizeAndModCount (with such repetition it's easier to introduce inconsistencies). 


> ---
> 
> Separate out hot and cold code in subListRangeCheck (although pre-existing code had the same problem)
> 
> +    static void subListRangeCheck(int fromIndex, int toIndex, int size) {
> +        if (fromIndex < 0)
> +            throw new IndexOutOfBoundsException("fromIndex = " + fromIndex);
> +        if (toIndex > size)
> +            throw new IndexOutOfBoundsException("toIndex = " + toIndex);
> +        if (fromIndex > toIndex)
> +            throw new IllegalArgumentException("fromIndex(" + fromIndex +
> +                                               ") > toIndex(" + toIndex + ")");
> +    }
> +
> 
> if ((fromIndex < 0) | (toIndex > size) | (fromIndex > toIndex)) slowpath();
> 
> ---
> java style consensus has been converging on: java source files should have exactly one top-level class.  It seems like you changed inner class SubList to be a top-level class - why?
> 
> +class ArraySubList<E> extends AbstractList<E> implements RandomAccess {
> 

I would usually opt for inner classes too, sometimes there is a good reason not to. I think for the sub-list of AbstractList it is easier to understand when it is not nested due to the confusion over what is the enclosing instance. For the sub-list of ArrayList there we could and probably should stick to usual the convention.

Paul.