On Wed, 12 Aug 2026 17:54:27 GMT, Bill Huang <[email protected]> wrote:
>> Changed Tests:
>>
>> HashMap/KeySetRemove.java: equal ValueClass keys mapped to null can be
>> removed from HashMap and TreeMap.
>>
>> HashMap/NullKeyAtResize.java: null-key resize behavior remains correct when
>> surrounding keys are ValueClass instances.
>>
>> HashMap/PutNullKey.java: colliding comparable tree-bin keys are
>> value-capable via @AsValueClass.
>>
>> HashMap/ReplaceExisting.java: replacing an existing ValueClass key during
>> active iteration does not corrupt the iterator.
>>
>> HashMap/SetValue.java: Map.Entry.setValue() returns the old ValueClass value.
>>
>> HashMap/ToArray.java: toArray() coverage is added for ValueClass keys,
>> values, and set elements across hash and linked variants.
>>
>> HashMap/TreeBinAssert.java: tree-bin iterator-removal coverage now uses a
>> value-capable key.
>>
>> Hashtable/EqualsCast.java: Provider/Hashtable equality now includes matching
>> ValueClass key/value entries.
>>
>> Hashtable/SimpleSerialization.java: serialization round-trip now also covers
>> Hashtable<Integer,Integer>.
>>
>> LinkedHashMap/ComputeIfAbsentAccessOrder.java: access-order behavior is
>> repeated with Integer keys.
>>
>> LinkedHashMap/EmptyMapIterator.java: fail-fast iterator behavior is repeated
>> with Integer key/value entries.
>>
>> LinkedList/AddAll.java: append-order behavior is repeated with ValueClass
>> elements.
>>
>> LinkedList/Clone.java: clone/equality checks for LinkedList, TreeSet, and
>> TreeMap subclasses now include ValueClass contents.
>>
>> Unchanged Tests:
>>
>> HashMap/HashMapCloneLeak.java: unchanged because the test depends on
>> WeakReference reachability. Value objects are not valid weak-reference
>> targets, so adding VClass would not match the regression being tested.
>>
>> HashMap/OverrideIsEmpty.java: unchanged because the test is about HashMap
>> subclass method dispatch. Value classes cannot extend HashMap, and changing
>> only the key/value payload would not add meaningful value-class coverage.
>>
>> HashMap/WhiteBoxResizeTest.java: unchanged because it is a
>> white-box/internal capacity and table-sizing test using
>> reflection/VarHandles over HashMap, LinkedHashMap, HashSet, and WeakHashMap
>> internals. Its purpose is sizing/lazy allocation/resize arithmetic, not
>> key/value equality or value-object behavior.
>>
>> HashMap/ToString.java: unchanged because it specifically verifies that
>> HashMap.Entry.toString() does not throw when the map contains null keys or
>> values. Adding value-class keys or values would not extend the original
>> null-handling regression in a meaningful ...
>
> Bill Huang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Implement review comment
I looked through the java/util/HashMap tests. I also looked through some
previous changes in java/util/Collections test changes that added the use of
VClass.
I see a LOT of duplication of test logic, that to my eye basically performs
exactly the same operations and checks. We need to put some effort into
avoiding and removing this duplication. Most of this is simply abstracting over
the types of things that are put into collections, but in the tests I looked at
nothing different was actually done with the VClass instances relative to the
original operations.
Maybe in test/lib have something like
interface Element { ... }
/* identity */ class IElement implements Element { ... }
@asValueClass class VElement implements Element { ... }
(The names probably need to be changed.)
Then, many of the test methods could be rewritten to take concrete type
Element, or in some cases they could have generic type args of <? extends
Element> or similar, as appropriate.
test/jdk/java/util/HashMap/PutNullKey.java line 109:
> 107: }
> 108:
> 109: return value == ((CollidingHashValue) o).value;
Can probably use `instanceof` here to avoid null check and cast in order to
extract `value`, and collapse into a single boolean expression.
test/jdk/java/util/HashMap/PutNullKey.java line 114:
> 112: @Override
> 113: public int compareTo(CollidingHashValue o) {
> 114: return value - o.value;
Use `Integer.compare` here. Subtraction won't fail given the int values used
here, but somebody might change it to use random ints.
Oh... I see you copied from the code above that does the same thing (and same
for equals). Is there some way to avoid this duplication?
test/jdk/java/util/HashMap/ReplaceExisting.java line 101:
> 99: }
> 100: }
> 101:
Was it necessary to change this file at all? Or... maybe some edits got lost
here or something?
test/jdk/java/util/HashMap/ToArray.java line 179:
> 177: checkToArray("Empty-tuple-keys", new VClass[0], map.keySet(),
> !ordered);
> 178: checkToArray("Empty-tuple-values", new VClass[0], map.values(),
> !ordered);
> 179: }
More duplication with checkMap. I think we need to put some effort into reusing
test logic between tests for identity classes and value classes.
Missing "checkVClassSet` ?
test/jdk/java/util/HashMap/TreeBinAssert.java line 234:
> 232: }
> 233: }
> 234: }
Again, a lot of addition of duplicate code.
-------------
Changes requested by smarks (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/32201#pullrequestreview-5024431462
PR Review Comment: https://git.openjdk.org/jdk/pull/32201#discussion_r3857623159
PR Review Comment: https://git.openjdk.org/jdk/pull/32201#discussion_r3857636906
PR Review Comment: https://git.openjdk.org/jdk/pull/32201#discussion_r3857647582
PR Review Comment: https://git.openjdk.org/jdk/pull/32201#discussion_r3857701662
PR Review Comment: https://git.openjdk.org/jdk/pull/32201#discussion_r3857728349
