On May 7, 2015, at 1:23 AM, Martin Buchholz <marti...@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.
