Hello! Here's the webrev: http://cr.openjdk.java.net/~tvaleev/webrev/8148748/r1/
I noticed that list == null checks are never true, thus redundant, so I removed them. The list field is final and always assigned to the outer ArrayList instance. So it seems quite natural to make ArrayListSpliterator non-static instead. Also I added one more field into ArrayListSpliterator to hold the SubList reference. Hopefully such footprint increase is acceptable. The offset field is initialized eagerly as it does not change during the SubList lifetime. Also I replaced "if (action == null)" checks with "Objects.requireNonNull". Some more thoughts about forEachRemaining: To me it seems that if ((a = lst.elementData) != null) is a redundant check as well as in current ArrayList implementation elementData is never null. So it can be replaced with simple assignment. Also this check: (i = index) >= 0 I don't see the case when i could be negative. Should it be removed as well? Or probably it's added to aid JIT to elimitate bounds-checking? With best regards, Tagir Valeev. PS> Hi Tagir, PS> Last-binding while adding some complexity to the implementation PS> is an important property, as it is to fail-fast on a best effort basis. PS> When one has mutable collections that can be structurally PS> modified and lazy views over those collections (Streams/Iterators) PS> then those views really need to act on the most up to date PS> structure and not a snapshot (structural or otherwise) when the PS> view was created. Otherwise, it is a potential source of bugs PS> producing incorrect results or exceptions, or another form of PS> complexity for the implementation to maintain integrity under such conditions. PS> We took a short-cut here to reuse the existing top-level PS> spliterator in a non-late-binding manner by specifying the PS> absolute fence. We should fix it. PS> Glancing at the code i believe there is a simple fix. When -1 is PS> passed as the fence to the ArrayList.ArrayListSpliterator then the PS> fence is created lazily and should be the result of offset + PS> list.size e.g. in the constructor: PS> hi = fence = offset + lst.size; PS> The forEach impl also needs updating. PS> — PS> Sublists are also tricky beasts. See documentation on List.subList: PS> * The semantics of the list returned by this method become undefined if PS> * the backing list (i.e., this list) is <i>structurally modified</i> in PS> * any way other than via the returned list. (Structural modifications are PS> * those that change the size of this list, or otherwise perturb it in such PS> * a fashion that iterations in progress may yield incorrect results.) PS> Paul. >> On 29 Jan 2016, at 05:32, Tagir F. Valeev <amae...@gmail.com> wrote: >> >> Hello! >> >> When reviewing 8079136 I noticed that >> ArrayList.subList().spliterator() is not late-binding: >> >> List<Integer> list = new ArrayList<>(Arrays.asList(1,2,3,4)).subList(0, 3); >> Stream<Integer> s = list.stream(); >> list.add(5); >> s.findFirst(); >> --> Exception in thread "main" java.util.ConcurrentModificationException >> >> This works correctly for other list implementation (for example, >> replacing ArrayList with LinkedList makes this code working). As >> Collection.spliterator() spec says: >> >>> In order to preserve expected laziness behavior for the stream() and >>> parallelStream() methods, spliterators should either have the >>> characteristic of IMMUTABLE or CONCURRENT, or be late-binding. If >>> none of these is practical, the overriding class should describe the >>> spliterator's documented policy of binding and structural >>> interference, and should override the stream() and parallelStream() >>> methods to create streams using a Supplier of the spliterator >> >> None of these is true for ArrayList.subList().spliterator(): it's not >> IMMUTABLE, not CONCURRENT, not late-binding, the binding policy is not >> documented and stream()/parallelStream() methods are not overridden. >> >> Is this an issue? >> >> Actually to my opinion late-binding spliterator behavior is a mistake. >> Nobody cares about it as nobody modifies the source between stream >> creation and terminal operation call. On the other hand, this >> requirement adds implementation complexity and I already found the >> second late-binding problem in existing code (the first one was with >> Pattern.splitAsStream). Also it's not documented that Stream.concat >> may bind the late-binding source, but nobody cares anyways. Life would >> be easier without late-binding spliterators. >> >> With best regards, >> Tagir Valeev. >> ===8<===========End of original message text=========== -- Best regards, Tagir mailto:amae...@gmail.com
