On Wed, 30 Apr 2025 09:15:49 GMT, Viktor Klang <vkl...@openjdk.org> wrote:
>> Chen Liang has updated the pull request with a new target base due to a >> merge or a rebase. The incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. The pull request contains 17 additional >> commits since the last revision: >> >> - Rewrite impl to follow the new simplified spec >> - Merge branch 'master' of https://github.com/openjdk/jdk into >> fix/classvalue-compute-remove >> - Try to simplify the model - use the finish of computeValue >> >> - Test updates - remove repetition, test case for no stale installation >> - Fix incorrect promise removal when unnecessary and add regression test >> - Cannot test for recursion eagerly - add test case >> - More spec, eager exception, finish with existing, thanks John >> - Merge branch 'master' of https://github.com/openjdk/jdk into >> fix/classvalue-compute-remove >> - docs >> - Remove the throwing behavior due to shallow reentrancy >> - ... and 7 more: https://git.openjdk.org/jdk/compare/153a25f7...221d51e7 > > src/java.base/share/classes/java/lang/ClassValue.java line 95: > >> 93: * <ul> >> 94: * <li>This association is already associated by another attempt. The >> 95: * associated value is returned.</li> > > It's a bit unclear which association is referred to in the second sentence, I > think that could be made clearer with something like: > > Suggestion: > > * <li>This association is already associated by another attempt, so that > value is returned.</li> I think I will reword as: "An association is already created by another attempt. The associated value is returned." What do you think? An association is either present or absent for a coordinate. In addition, all other list entries are "What happened. What happens next." > src/java.base/share/classes/java/lang/ClassValue.java line 124: > >> 122: * {@return the value associated to the given {@code Class}} >> 123: * <p> >> 124: * This method first performs a read-only access, and returns the >> associated > > I'm not 100% sure on the "read-only" label here, as it does > recompute-and-store if there is no current value. Yep, it needs a read-only access to know there is no current value... > src/java.base/share/classes/java/lang/ClassValue.java line 167: > >> 165: public void remove(Class<?> type) { >> 166: ClassValueMap map = getMap(type); >> 167: map.removeAccess(this); > > Should this really be "Access" and not "Association"? Oh, I named all methods on `ClassValueMap` to `access`. When a remove is done, it might remove a previous remove (no association) in addition to actual associations. > src/java.base/share/classes/java/lang/ClassValue.java line 318: > >> 316: Version<T> version() { return version; } >> 317: void bumpVersion() { version = new Version<>(this); } >> 318: static final class Version<T> { > > Since this is all final components, it might serve better as a record? Note that this class has a significant identity. Does that make it still suitable for record conversion? I think we need a comment on its identity significance if we convert. > test/jdk/java/lang/invoke/ClassValueTest.java line 179: > >> 177: >> 178: private static final long COMPUTE_TIME_MILLIS = 100; >> 179: private static final Duration TIMEOUT = Duration.of(2, >> ChronoUnit.SECONDS); > > In order to reduce spurious failures here, I'd probably put this timeout a > bit longer—let's say 5s. Oh, the timeout is not to reduce failures - it mainly aims to prevent accidental infinite loops from stalling tests. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/24043#discussion_r2069106581 PR Review Comment: https://git.openjdk.org/jdk/pull/24043#discussion_r2069108612 PR Review Comment: https://git.openjdk.org/jdk/pull/24043#discussion_r2069109848 PR Review Comment: https://git.openjdk.org/jdk/pull/24043#discussion_r2069113085 PR Review Comment: https://git.openjdk.org/jdk/pull/24043#discussion_r2069114518
