RFR: 8221473: Configuration::reads can use Set.copyOf

[Peter Levart]

Hi,

This change is OK, but I was thinking that in general, for cases like 
this one, it would be better to check that the Set obtained from the 
'graph' Map is already immutable and throw an exception if that is not 
the case. Like Objects.requireNonNull method, there could be a similar 
static method in Set interface:

static <T> Set<T> requireImmutable(Set<T> set)

Why is this better than Set.copyOf()? Because here we know that the 
parameter to Set.copyOf() is already an immutable Set and this method 
just returns it in that case. But what if some changes to internal logic 
fail to maintain this invariant? In that case Set.copyOf() would 
silently start copying the elements and returning new immutable instances.

In case that happens, Set.requireImmutable() would throw and it would be 
necessary to fix code so that the invariant is preserved or replace 
Set.requireImmutable() with Set.copyOf(). But that last option would be 
a conscious decision which trades correctness for performance, not a 
result of oversight.

Would such method addition be worth it?

Regards, Peter

On 3/26/19 2:54 PM, Alan Bateman wrote:
> On 26/03/2019 13:44, Claes Redestad wrote:
>>
>> Or with this less verbose comment (suggested offline by Alan):
>>
>> diff -r 5ee30b6991a7 
>> src/java.base/share/classes/java/lang/module/Configuration.java
>> --- a/src/java.base/share/classes/java/lang/module/Configuration.java 
>> Mon Dec 03 16:25:27 2018 +0100
>> +++ b/src/java.base/share/classes/java/lang/module/Configuration.java 
>> Tue Mar 26 14:50:55 2019 +0100
>> @@ -575,7 +575,8 @@
>>      }
>>
>>      Set<ResolvedModule> reads(ResolvedModule m) {
>> -        return Collections.unmodifiableSet(graph.get(m));
>> +        // The sets stored in the graph are already immutable sets
>> +        return Set.copyOf(graph.get(m));
>>      }
> This looks good.
>
> -Alan