On Fri, 18 Apr 2025 13:19:05 GMT, Jorn Vernee <jver...@openjdk.org> wrote:
>> Per Minborg has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Address comments on original vs underlying
>
> src/java.base/share/classes/java/lang/StableValue.java line 454:
>
>> 452: * @param content to set
>> 453: * @throws IllegalStateException if this method is invoked directly
>> by a supplier
>> 454: * provided to the {@link #orElseSet(Supplier)} method.
>
> Why does this result in an exception? Given the example of dependent SV's I
> thought it would be fine or e.g. a stable value A to initialize another
> stable value B using `trySet`.
I see now the exception specification for `orElseSet`, I suggest saying
something similar here:
Suggestion:
* @throws IllegalStateException if a supplier invoked by {@link
#orElseSet(Supplier)} recursively
* attempts to set this stable value by calling this method.
> src/java.base/share/classes/jdk/internal/lang/stable/StableValueImpl.java
> line 173:
>
>> 171: private void preventReentry() {
>> 172: if (Thread.holdsLock(this)) {
>> 173: throw new IllegalStateException("Recursive initialization
>> is not supported");
>
> Prevent users from having to look through the stack trace. Also, 'supported'
> sounds like it might be implemented later. I think 'allowed' is better to
> signal that this is an invariant.
>
> Suggestion:
>
> throw new IllegalStateException("Recursive initialization of a
> StableValue is not allowed");
Or maybe a message like "Illegal recursive initialization of stable value".
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2050625500
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2050761523
