Re: RFR(m): 8159404: immutable collections should throw UOE unconditionally

[Stuart Marks]

On 9/1/16 8:41 PM, Martin Buchholz wrote:

Looks good to me!

Thanks!
Another idea for another day: I would like the immutable collections to be 
more optimal than they currently are, even if we have to write more code.  It 
bugs me is that all of these collections have a modCount, despite never being 
modified!  (Even for mutable lists, I'm not sure the added safety of modCount 
was worth the price)


java -jar jol-cli-0.5-full.jar internals java.util.ImmutableCollections\$List1
...
java.util.ImmutableCollections$List1 object internals:
  012(object header)N/A
 12 4int AbstractList.modCount  N/A
 16 4 Object List1.e0   N/A

Yes, good idea. I've filed JDK-8165396 to track this.

s'marks

Re: RFR(m): 8159404: immutable collections should throw UOE unconditionally

[Paul Sandoz]

> On 1 Sep 2016, at 20:41, Martin Buchholz  wrote:
> 
> Looks good to me!
> 

+1

Paul.

Re: RFR(m): 8159404: immutable collections should throw UOE unconditionally

[Martin Buchholz]

Looks good to me!

Another idea for another day: I would like the immutable collections to be
more optimal than they currently are, even if we have to write more code.
It bugs me is that all of these collections have a modCount, despite never
being modified!  (Even for mutable lists, I'm not sure the added safety of
modCount was worth the price)

java -jar jol-cli-0.5-full.jar internals
java.util.ImmutableCollections\$List1
...
java.util.ImmutableCollections$List1 object internals:
  012(object header)N/A
 12 4int AbstractList.modCount  N/A
 16 4 Object List1.e0   N/A




On Thu, Sep 1, 2016 at 6:47 PM, Stuart Marks 
wrote:

> Hi all,
>
> Please review this change to make the immutable collections (List.of,
> Set.of, Map.of) throw UnsupportedOperationException unconditionally. That
> is, they should throw UOE even if a mutator method is called with arguments
> that make it a no-op. Calling a mutator method on an immutable collection
> is always a programming error, and having it sometimes be a no-op
> potentially leads to errors.
>
> Note that the existing Collections unmodifiable wrappers always throw UOE
> unconditionally. This change makes the immutable collections behave
> consistently with the unmodifiable wrappers. For example,
>
> List unmodList = Collections.unmodifiableList(new
> ArrayList<>());
> unmodList.addAll(List.of()); // throws UOE
> List.of().addAll(List.of()); // currently does nothing, change to
> throw UOE
>
> Unfortunately, various other specialized collections such as emptyList()
> etc. behave differently, e.g.
>
> Collections.emptyList().addAll(List.of()); // does nothing
>
> However, that change will be left for another day.
>
> Bug:
>
> https://bugs.openjdk.java.net/browse/JDK-8159404
>
> Webrev:
>
> http://cr.openjdk.java.net/~smarks/reviews/8159404/webrev.0/
>
> Thanks,
>
> s'marks
>

RFR(m): 8159404: immutable collections should throw UOE unconditionally

[Stuart Marks]

Hi all,

Please review this change to make the immutable collections (List.of, Set.of, 
Map.of) throw UnsupportedOperationException unconditionally. That is, they 
should throw UOE even if a mutator method is called with arguments that make it 
a no-op. Calling a mutator method on an immutable collection is always a 
programming error, and having it sometimes be a no-op potentially leads to errors.


Note that the existing Collections unmodifiable wrappers always throw UOE 
unconditionally. This change makes the immutable collections behave consistently 
with the unmodifiable wrappers. For example,


List unmodList = Collections.unmodifiableList(new ArrayList<>());
unmodList.addAll(List.of()); // throws UOE
List.of().addAll(List.of()); // currently does nothing, change to throw UOE

Unfortunately, various other specialized collections such as emptyList() etc. 
behave differently, e.g.


Collections.emptyList().addAll(List.of()); // does nothing

However, that change will be left for another day.

Bug:

https://bugs.openjdk.java.net/browse/JDK-8159404

Webrev:

http://cr.openjdk.java.net/~smarks/reviews/8159404/webrev.0/

Thanks,

s'marks