> On 4 Mar 2016, at 17:42, Tagir F. Valeev <amae...@gmail.com> wrote:
>
> Hello!
>
> AIOOBE is possible for ArrayList itself as well. E.g.:
>
> ArrayList<Integer> test = new ArrayList<>(Arrays.asList(1,2,3,4));
> Spliterator<Integer> spltr = test.spliterator();
> spltr.tryAdvance(System.out::println);
> test.clear();
> test.trimToSize();
> spltr.tryAdvance(System.out::println);
>
> Result (both in Java-8 and Java-9):
>
> 1
> Exception in thread "main" java.lang.ArrayIndexOutOfBoundsException: 1
> at java.util.ArrayList$ArrayListSpliterator.tryAdvance(ArrayList.java:1398)
>
> So this is not subList-specific problem.
>
Yes, that was the first aspect i mentioned. Generally for bulk traversal we
opted to throw the CME at the end, which i think is a reasonable compromise,
especially since mixed traversal is an edge case. I was not suggesting we
revisit that.
I was concerned about the sublist case, then I recalled a co-mod check is
performed *before* construction:
1282 public Spliterator<E> spliterator() {
1283 checkForComodification();
1284
1285 // ArrayListSpliterator is not used because late-binding logic
1286 // is different here
1287 return new Spliterator<>() {
I forgot about that! sorry for the noise. We are good. I will push on Monday.
Thanks,
Paul.
> Seems that forEachRemaining is not affected by this, due to additional
> length check and making the local copy of elementData array. At least
> I don't see the way to make forEachRemaining (either of subList
> spliterator or main ArrayList spliterator) throwing AIOOBE. Probably
> I'm missing something. However it can traverse some unexpected nulls
> before throwing CME if it was shrinked:
>
> ArrayList<Integer> test = new ArrayList<>(Arrays.asList(1,2,3,4));
> Spliterator<Integer> spltr = test.spliterator();
> spltr.tryAdvance(System.out::println);
> test.clear();
> spltr.forEachRemaining(System.out::println);
>
> Output:
>
> 1
> null
> null
> null
> Exception in thread "main" java.util.ConcurrentModificationException
> at
> java.util.ArrayList$ArrayListSpliterator.forEachRemaining(ArrayList.java:1428)
>
> This may unexpectedly throw NPE if forEachRemaining Consumer
> unconditionally dereference them as program logic suggests that null
> is impossible here. So user will see NPE instead of CME.
>
> Probably it could be logged as separate issue. My patch does not cause
> any regression here (at least, I hope so). Probably some other
> collections should be checked for similar corner cases as well.
>
> With best regards,
> Tagir Valeev.
>
>>> On 4 Mar 2016, at 15:38, Tagir F. Valeev <amae...@gmail.com> wrote:
>>>
>>> Hello!
>>>
>>> Thank you for the review!
>>>
>
> PS> Thanks.
>
> PS> I just realised there are some subtleties where if the top-level
> PS> list is reduced in size the spliterator of a sublist may on
> PS> traversal throw an ArrayIndexOutOfBoundsException rather than
> ConcurrentModificationException.
>
> PS> This can already occur for a partially traversed top-level list
> PS> spliterator, so i am wondering how much we should care. Arguably
> PS> in the sublist case there is another level of indirection, where
> PS> errors creep in before traversal, so it suggests we should
> PS> additionally check the mod count at the start of the traversal
> PS> methods, and it’s probably ok on a best-effort basis to do this
> PS> for the sublist spliterator and not it’s splits.
>
> PS> Separately, i am not sure why the SubList.iterator has to check
> PS> that the offset + 1 is within bounds, since expectedModCount =
> PS> ArrayList.this.modCount should be false.
>
> PS> Paul.
>
>>> Here's updated webrev:
>>> http://cr.openjdk.java.net/~tvaleev/webrev/8148748/r3/
>>>
>>> PS> Looks good. I especially like:
>>>
>>> PS> 125 addCollection(l.andThen(list -> list.subList(0,
>>> list.size())));
>>>
>>> PS> Can you also update SpliteratorTraversingAndSplittingTest?
>>>
>>> PS> void addList(Function<Collection<T>, ? extends List<T>> l) {
>>> PS> // @@@ If collection is instance of List then add sub-list tests
>>> PS> addCollection(l);
>>> PS> }
>>>
>>> Done.
>>>
>>>>> Now it's separate anonymous class as you suggested.
>>>>> ArrayListSpliterator is untouched. Note that trySplit() can return
>>>>> original ArrayListSpliterator as after the binding their behavior is
>>>>> compatible.
>>>>>
>>>
>>> PS> Very nice, might be worth an extra comment noting that. Up to you.
>>>
>>> Short comments added.
>>>
>>> With best regards,
>>> Tagir Valeev.
>>>
>
