On 24 Jul 2015, at 20:30, Martin Buchholz <martinrb@google.com> wrote:

Hi Steve, pleased to meet you!

(are you a new maintainer of java.util collections?)

I suggested that Steve look at some “simple” issues to build up credits for a committer role, so i threw this and some others over the fence for Steve to review and possibly fix.

I'm mildly opposed to this change - I added comments to the bug.

It looks like you are not the only one. I am outnumbered :-) My guiding principle here was that argument validation should not result in side-effects. Thus the state of a collection should remain unchanged if an exception is thrown due to an invalid argument of an operation. ArrayList.remove is inconsistent with regards to nearly all the other ArrayList methods, add(int, E) and addAll(int, Collection) for an out of bounds index, and addAll(Collection ), removeAll, retainAll, removeIf, replaceAll and sort for a null argument. It’s not inconsistent with ArrayList.removeRange, except for if an invalid range from > t, but is inconsistent with AbstractList.removeRange. It’s also inconsistent LinkedList and CopyOnWriteArrayList *and* sub-lists of ArrayList and AbstractList. And inconsistent more generally for other Collection implementations, such as Queue.add/offer/remove implementations that do not accept null values. The behaviour of ArrayList.remove, and also ArrayList.removeRange, are outliers [*]. It’s also a rather obscure difference behaviour with likely minimal impact if changed (far less so than the change to Arrays.toList) . Paul. [*] While there is a argument to be had for updating concurrent modification state of a collection for an operation that performs structural modification regardless of whether its arguments are valid or not such a behavioural change would have far more impact.

On Fri, Jul 24, 2015 at 11:16 AM, Steve Drach <steve.drach@oracle.com> wrote:

...

Hello,

Please review the fix for JDK-8114832. I did what was suggested in the comments in the bug report, moving the increment of modcount to the “right” place.

https://bugs.openjdk.java.net/browse/JDK-8114832 < https://bugs.openjdk.java.net/browse/JDK-8114832> http://slc01hfj.us.oracle.com/webrevs/JDK-8114832/webrev/index.html < http://slc01hfj.us.oracle.com/webrevs/JDK-8114832/webrev/index.html>

An internal review has been completed.

Thanks, Steve

On 27 Jul 2015, at 20:53, Martin Buchholz <martinrb@google.com> wrote:

On Mon, Jul 27, 2015 at 1:19 AM, Paul Sandoz <paul.sandoz@oracle.com> wrote:

My guiding principle here was that argument validation should not result in side-effects. Thus the state of a collection should remain unchanged if an exception is thrown due to an invalid argument of an operation.

That is indeed a good principle. All or nothing!

But in the case of modCount, one might consider it to record both actual and attempted modifications. I would be OK if we decided to make this consistent across all JDK implementations. Currently we don't have any internal policy on this, although you can read the spec https://docs.oracle.com/javase/8/docs/api/java/util/AbstractList.html#modCou... as requiring successful, not attempted modifications.

I just implicitly assumed that was always the case :-) as i assume did many core library implementors.

ArrayList.remove is inconsistent with regards to nearly all the other ArrayList methods, add(int, E) and addAll(int, Collection) for an out of bounds index, and addAll(Collection ), removeAll, retainAll, removeIf, replaceAll and sort for a null argument. It’s not inconsistent with ArrayList.removeRange, except for if an invalid range from > t, but is inconsistent with AbstractList.removeRange. It’s also inconsistent LinkedList and CopyOnWriteArrayList *and* sub-lists of ArrayList and AbstractList. And inconsistent more generally for other Collection implementations, such as Queue.add/offer/remove implementations that do not accept null values.

The behaviour of ArrayList.remove, and also ArrayList.removeRange, are outliers [*]. It’s also a rather obscure difference behaviour with likely minimal impact if changed (far less so than the change to Arrays.toList) .

I agree that if we make a policy decision here, we can change it and the compatibility impact is minimal.

Since there already exists an implicit policy governed by like 99.9% of the existing behaviour i do not see the need to be explicit about that policy for this particular issue, so i suggest we split into two. Paul.

On 31 Jul 2015, at 04:47, Stuart Marks <stuart.marks@oracle.com> wrote:

On 7/28/15 5:50 AM, Paul Sandoz wrote:

...

...

I agree that if we make a policy decision here, we can change it and the compatibility impact is minimal.

Since there already exists an implicit policy governed by like 99.9% of the existing behaviour i do not see the need to be explicit about that policy for this particular issue, so i suggest we split into two.

I followed this thread up until the last sentence. What's the resolution here?

I think we should go ahead with the fix (and file a CCC just for formality), the file another issue for clarifying the current implicit behaviour. Paul.