On Wed, 2 Apr 2025 09:13:32 GMT, Maurizio Cimadamore <[email protected]>
wrote:
>> Per Minborg has updated the pull request incrementally with two additional
>> commits since the last revision:
>>
>> - Add lazy toSting for StableMap::values
>> - Make toString for reversed and sublist lazy
>
> src/java.base/share/classes/java/lang/StableValue.java line 371:
>
>> 369:
>> 370: /**
>> 371: * {@return {@code true} if the content was set to the provided
>> {@code value},
>
> I find this description a bit confusing -- as written it almost looks like a
> predicate -- e.g. test if this stable value is set with the value I'm
> providing. But the important thing here is the side-effect -- so I feel the
> javadoc for this method should be much less about what this method returns,
> and much more about what the method actually does. (`setOrThrow` seems to be
> better in this respect)
I also note that, so far, we have always referred to the thing contained in a
stable value as "content" (bonus points for that -- as that's also the
terminology we settled on in the JEP!). But here (and in other methods) we fall
back to `value` -- which is generic, and also a bit confusing with respect to
`StableValue` itself.
> src/java.base/share/classes/java/lang/StableValue.java line 416:
>
>> 414: * When this method returns successfully, the content is always set.
>> 415: * <p>
>> 416: * This method will always return the same witness value regardless
>> if invoked by
>
> will always return/always returns (more direct)
`witness value` -- what is that? Also, why do we say that here -- does it mean
that `orElseThrow` can return different values? Overall I'm not sure what this
last section wants to add that wasn't already covered at the beginning by the
very clear:
> The provided {@code supplier} is guaranteed to be invoked at most once if it
> completes without throwing an exception.
> src/java.base/share/classes/java/lang/StableValue.java line 461:
>
>> 459: * An unset stable value has no content.
>> 460: * <p>
>> 461: * The returned stable value is not {@link Serializable}.
>
> I find the claim about serializable a bit odd here. Perhaps in some
> `implSpec`/`apiNote` at the toplevel?
Same in other factories...
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2024427669
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2024436050
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2024448413
