On Thu, 13 Mar 2025 15:22:43 GMT, Per Minborg <pminb...@openjdk.org> wrote:
>> Implement JEP 502. >> >> The PR passes tier1-tier3 tests. > > Per Minborg has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 246 commits: > > - Merge branch 'master' into implement-jep502 > - Clean up exception messages and fix comments > - Rename field > - Rename method and fix comment > - Rework reenterant logic > - Use acquire semantics for reading rather than volatile semantics > - Add missing null check > - Simplify handling of sentinel, wrap, and unwrap > - Fix JavaDoc issues > - Fix members in StableEnumFunction > - ... and 236 more: https://git.openjdk.org/jdk/compare/4e51a8c9...d6e1573f src/java.base/share/classes/java/util/ImmutableCollections.java line 798: > 796: throw new IndexOutOfBoundsException(i); > 797: } > 798: } I think `orElseSet` should be outside of the `try` block, otherwise an `ArrayIndexOutOfBoundsException` thrown by `mapper.apply` will be wrapped. src/java.base/share/classes/java/util/ImmutableCollections.java line 1488: > 1486: final K k = (K) key; > 1487: return stable.orElseSet(new Supplier<V>() { > 1488: @Override public V get() { return mapper.apply(k); }}); This can return `null` (`StableMap` does allow `null` values), so the `getOrDefault` implementation in `AbstractImmutableMap` does not properly work for `StableMap`: var map = StableValue.map(Set.of(1), _ -> null); // should print "null", but prints "default value" System.out.println(map.getOrDefault(1, "default value")); src/java.base/share/classes/jdk/internal/lang/stable/EmptyStableFunction.java line 47: > 45: @Override > 46: public R apply(T value) { > 47: throw new IllegalArgumentException("Input not allowed: " + value); `StableEnumFunction` and `StableFunction` throw a `NullPointerException` if `value` is `null`. src/java.base/share/classes/jdk/internal/lang/stable/StableEnumFunction.java line 68: > 66: } catch (ArrayIndexOutOfBoundsException ioob) { > 67: throw new IllegalArgumentException("Input not allowed: " + > value, ioob); > 68: } Same here. src/java.base/share/classes/jdk/internal/lang/stable/StableIntFunction.java line 61: > 59: throw new IllegalArgumentException("Input not allowed: " + > index, ioob); > 60: } > 61: } Same here. src/java.base/share/classes/jdk/internal/lang/stable/StableValueFactories.java line 77: > 75: int i = 0; > 76: for (K key : keys) { > 77: entries[i++] = Map.entry(key, StableValueImpl.newInstance()); `Map.entry` causes `null` keys to throw a `NullPointerException`, meaning there can't be stable functions/maps with a `null` input/key. They can however have `null` values. Is that intended? src/java.base/share/classes/jdk/internal/lang/stable/StableValueImpl.java line 132: > 130: // Prevent reentry > 131: if (Thread.holdsLock(this)) { > 132: throw new IllegalStateException("Recursing supplier > detected: " + supplier); Is it right to include `supplier` in the message? The actual recursing supplier could be another one: var s = StableValue.<Integer>of(); // throws java.lang.IllegalStateException: Recursing supplier detected: supplier 2 s.orElseSet(new Supplier<>() { public String toString() {return "supplier 1";} public Integer get() { s.orElseSet(new Supplier<>() { public String toString() {return "supplier 2";} public Integer get() {return 2;} }); return 1; } }); The exception message could also be confusing in cases like this: var s = StableValue.of(); synchronized (s) { // throws java.lang.IllegalStateException: Recursing supplier detected s.trySet(null); } ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r1997660330 PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r1997685354 PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r1997681440 PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r1997660665 PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r1997660937 PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r1997680982 PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r1997689245
