RFR: 8356080: Address post-integration comments for Stable Values [v3]

[Luca Kellermann]

On Mon, 12 May 2025 07:42:16 GMT, Per Minborg <pminborg at 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 <liach at 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