On Fri, 2 May 2025 12:13:39 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.
>
> Unfortunately, this PR shows the total commit history for stable values.
src/java.base/share/classes/java/util/ImmutableCollections.java line 895:
> 893: public List<E> subList(int fromIndex, int toIndex) {
> 894: int size = size();
> 895: subListRangeCheck(fromIndex, toIndex, size);
Suggestion:
subListRangeCheck(fromIndex, toIndex, size());
size is only used once, can it be simplified like this?
src/java.base/share/classes/java/util/ImmutableCollections.java line 905:
> 903: // Todo: Provide a copy free solution
> 904: final StableValueImpl<E>[] reversed =
> Arrays.copyOfRange(delegates, this.offset, this.size - offset);
> 905: return StableUtil.renderElements(this,
> "StableCollection", reversed);
Suggestion:
return StableUtil.renderElements(this, "StableCollection",
Arrays.copyOfRange(deepRoot(root).delegates, this.offset, this.size - offset));
The three local variables `deepRoot/delegates/reversed` are only used once. Can
they be simplified like this?
src/java.base/share/classes/java/util/ImmutableCollections.java line 932:
> 930: final StableValueImpl<E>[] reversed =
> ArraysSupport.reverse(
> 931: Arrays.copyOf(delegates, delegates.length));
> 932: return StableUtil.renderElements(this, "Collection",
> reversed);
Suggestion:
final StableValueImpl<E>[] delegates = deepRoot(base).delegates;
// Todo: Provide a copy free solution
return StableUtil.renderElements(this, "Collection",
ArraysSupport.reverse(Arrays.copyOf(delegates, delegates.length)));
Same as above, simplifying the code by removing local variables that are only
used once
src/java.base/share/classes/jdk/internal/lang/stable/StableSupplier.java line
62:
> 60: public String toString() {
> 61: final Object t = delegate.wrappedContentsAcquire();
> 62: return t == this ? "(this StableSupplier)" :
> StableValueImpl.renderWrapped(t);
Suggestion:
return delegate.wrappedContentsAcquire() == this
? "(this StableSupplier)"
: StableValueImpl.renderWrapped(t);
Same as above, simplifying the code by removing local variables that are only
used once
src/java.base/share/classes/jdk/internal/lang/stable/StableValueImpl.java line
54:
> 52: // Unsafe offsets for direct field access
> 53:
> 54: private static final long CONTENT_OFFSET =
It should be CONTENTS_OFFSET, right?
src/java.base/share/classes/jdk/internal/lang/stable/StableValueImpl.java line
108:
> 106: if (t == null) {
> 107: throw new NoSuchElementException("No contents set");
> 108: }
Suggestion:
if (wrappedContentsAcquire() == null) {
throw new NoSuchElementException("No contents set");
}
Same as above, simplifying the code by removing local variables that are only
used once
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/25004#discussion_r2071635586
PR Review Comment: https://git.openjdk.org/jdk/pull/25004#discussion_r2071642599
PR Review Comment: https://git.openjdk.org/jdk/pull/25004#discussion_r2071648603
PR Review Comment: https://git.openjdk.org/jdk/pull/25004#discussion_r2071651874
PR Review Comment: https://git.openjdk.org/jdk/pull/25004#discussion_r2071654313
PR Review Comment: https://git.openjdk.org/jdk/pull/25004#discussion_r2071656349
