RFR(m): 8177290 add copy factory methods for unmodifiable List, Set, Map

[Stuart Marks]

Thanks for the review. Editorial comments accepted. Discussion and responses to some items below.


On 10/31/17 7:25 AM, Brian Goetz wrote:
>
> Returns an <a href="#unmodifiable">unmodifiable Map</a> containing the entries
> 1664 * of the given Map. the given Map must not be null, and it must not 
> contain any
> 1665 * null keys or values. If the given Map is subsequently modified, the 
> returned
> 1666 * Map will not reflect such modifications.
>
> I might quibble with "subsequently modified", as there is some ambiguity (what 
> if the map is concurrently modified, subsequent to making the copyOf() call, 
> but prior to the return from copyOf()?)  But I won't ;)
>
Well I actually did think about this, but I couldn't think of anything better to 
say. If concurrent modification is permitted, it's conceivable that a 
modification made any time during the copy might end up in the returned map. It 
depends on the source map's implementation and concurrent modification policy. 
So I think the only statement that can be made is about the state of the 
returned map after copyOf() returns. That's what "subsequent" means to me, but I 
could clarify this if you think it's worth it.

(Similar reasoning for List and Set.)
>
> 296 Collector<T, ?, List<T>> toUnmodifiableList() {
> 297 return new CollectorImpl<>((Supplier<List<T>>) ArrayList::new, List::add,
> 298 (left, right) -> { left.addAll(right); return left; },
> 299 list -> (List<T>)List.of(list.toArray()),
> 300 CH_NOID);
> why the intermediate array, and not just use copyOf()?  (Is it because you 
> added copyOf later and didn't go back and update this?)  Same for other 
> collectors.
The copyOf method is mostly forced to make a defensive copy, so it's no better.

I think what should happen here eventually is that the elements should be 
collected into something like a SpinedBuffer, and then a private interface 
should be plumbed into the unmodifiable list implementation that can copy 
elements directly from the buffer. This avoids the extra copy. But I'll take 
care of that later.

For toUnmodifiableSet and toUnmodifiableMap, I can imagine similar private 
interfaces to avoid excess copies, but I think they each still need to build up 
full intermediate HashSet/HashMap in order to get rid of duplicates.

s'marks
>
>
>
>
>
>
>
>
> On 10/30/2017 6:50 PM, Stuart Marks wrote:
>> (also includes 8184690: add Collectors for collecting into unmodifiable List, 
>> Set, and Map)
>>
>> Hi all,
>>
>> Here's an updated webrev for this changeset; the previous review thread is here:
>>
>> http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-September/049261.html
>>
>> This webrev includes the following:
>>
>> * specification revisions to provide clearer definitions of "view" 
>> collections, "unmodifiable" collections, and "unmodifiable views"
>>
>> * new List.copyOf(), Set.copyOf(), and Map.copyOf() "copy factory" methods
>>
>> * new Collectors.toUnmodifiableList, Set, and Map methods
>>
>> * tests for the new API methods
>>
>> I've added some assertions that require some independence between the source 
>> collection (or map) and the result of the copyOf() method.
>>
>> I've made a small but significant change to Set.copyOf compared to the 
>> previous round. Previously, it specified that the first of any equal elements 
>> was preserved. Now, it is explicitly unspecified which of any equals elements 
>> is preserved. This is consistent with Set.addAll, Collectors.toSet, and the 
>> newly added Collectors.toUnmodifiableSet, none of which specify which of 
>> duplicate elements is preserved.
>>
>> (The outlier here is Stream.distinct, which specifies that the first element 
>> of any duplicates is preserved, if the stream is ordered.)
>>
>> I've also made some minor wording/editorial changes in response to 
>> suggestions from David Holmes and Roger Riggs. I've kept the wording changes 
>> that give emphasis to "unmodifiable" over "immutable." The term "immutable" 
>> is inextricably intertwined with "persistent" when it comes to data 
>> structures, and I believe we'll be explaining this forever if Java's 
>> "immutable" means something different from everybody else's.
>>
>> Webrev:
>>
>> http://cr.openjdk.java.net/~smarks/reviews/8177290/webrev.1/
>>
>> Bugs:
>>
>> https://bugs.openjdk.java.net/browse/JDK-8177290
>>         add copy factory methods for unmodifiable List, Set, Map
>>
>> https://bugs.openjdk.java.net/browse/JDK-8184690
>>         add Collectors for collecting into unmodifiable List, Set, and Map
>>
>> Thanks,
>>
>> s'marks
>