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
