On Mon, 12 May 2025 07:42:16 GMT, Per Minborg <pminb...@openjdk.org> wrote:
>> This PR proposes to address comments in the initial PR for Stable Values, >> which were deferred until after integration. >> >> Most of the efforts in this PR are to retain "stability" as long as possible >> so that views of stable collections remain stable and do not evaluate on >> `toString()` for example. >> >> Unfortunately, this PR shows the total commit history for stable values. > > Per Minborg has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 386 commits: > > - Address comments > - Merge branch 'master' into jep502-followup > - Fix an issue with toString on nested constructs > - Merge branch 'master' into jep502-followup > - Merge branch 'master' into jep502-followup > - Update src/java.base/share/classes/java/lang/StableValue.java > > Co-authored-by: Chen Liang <li...@openjdk.org> > - Simplify furhter > - Address comments in PR > - Merge master > - Remove unused method and add comment > - ... and 376 more: https://git.openjdk.org/jdk/compare/43008b42...a2826336 src/java.base/share/classes/java/lang/StableValue.java line 730: > 728: * <p> > 729: * Any {@link Map#values()} or {@link Map#entrySet()} views of the > returned map are > 730: * also stable. You resolved https://github.com/openjdk/jdk/pull/23972#discussion_r2058903119, so the following lines can also get rid of "direct": https://github.com/openjdk/jdk/blob/fbc4691bfa11f31601fd89d05da63e689343e214/src/java.base/share/classes/java/lang/StableValue.java#L687-L688 src/java.base/share/classes/java/util/ImmutableCollections.java line 518: > 516: > 517: private boolean allowNulls() { > 518: return root instanceof ListN<?> listN && listN.allowNulls; I think this should stay here (or some other change is needed). `StableSubList` doesn't override `indexOf` and `lastIndexOf`, so they will throw when given `null`, even though the list could contain `null`. src/java.base/share/classes/java/util/ImmutableCollections.java line 884: > 882: @Override > 883: public String toString() { > 884: return StableUtil.renderElements(this, "StableList", > delegates); Suggestion: return StableUtil.renderElements(this, "StableCollection", delegates); I think `StableList`, `StableSubList`, `StableReverseOrderListView`, `StableMapEntrySet` and `StableMapValues` should all use `"StableCollection"` to be consistent with `AbstractCollection`'s use of `"Collection"`. `StableMap` should use `"StableMap"` to be consistent with `AbstractMap`'s use of `"Map"`. src/java.base/share/classes/java/util/ImmutableCollections.java line 1660: > 1658: @Override > 1659: public String toString() { > 1660: return StableUtil.renderMappings(this, "StableSet", > delegateEntrySet, false); Suggestion: return StableUtil.renderMappings(this, "StableCollection", delegateEntrySet, false); I think `StableList`, `StableSubList`, `StableReverseOrderListView`, `StableMapEntrySet` and `StableMapValues` should all use `"StableCollection"` to be consistent with `AbstractCollection`'s use of `"Collection"`. `StableMap` should use `"StableMap"` to be consistent with `AbstractMap`'s use of `"Map"`. src/java.base/share/classes/jdk/internal/lang/stable/StableEnumFunction.java line 70: > 68: } > 69: final int index = value.ordinal() - firstOrdinal; > 70: final StableValueImpl<R> delegate; Suggestion: This local variable is no longer used. src/java.base/share/classes/jdk/internal/lang/stable/StableValueImpl.java line 57: > 55: UNSAFE.objectFieldOffset(StableValueImpl.class, "contents"); > 56: > 57: // Used to indicate a holder value is `null` (see field `value` below) Suggestion: // Used to indicate a holder value is `null` (see field `contents` below) The field is named `contents`. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/25004#discussion_r2085444718 PR Review Comment: https://git.openjdk.org/jdk/pull/25004#discussion_r2085348072 PR Review Comment: https://git.openjdk.org/jdk/pull/25004#discussion_r2085372556 PR Review Comment: https://git.openjdk.org/jdk/pull/25004#discussion_r2085422959 PR Review Comment: https://git.openjdk.org/jdk/pull/25004#discussion_r2085265954 PR Review Comment: https://git.openjdk.org/jdk/pull/25004#discussion_r2085289780
