On Tue, 4 Aug 2026 16:31:16 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 way.
>
> HashSet/Serialization.java: unchanged because the existing HashSet s...
There are a lot of additions; many seems to be testing feature disjoint from
value objects. @stuart-marks should check out too.
test/jdk/java/util/HashMap/KeySetRemove.java line 50:
> 48: valueMaps[i].put(new VClass(1, new int[] { 1 }), null);
> 49: if (!valueMaps[i].keySet().remove(new VClass(1, new int[] { 1
> })))
> 50: throw new Exception("Value banana was not removed: " + i);
Suggestion:
throw new Exception("VClass{1, [1]} was not removed: " + i);
`banana` was the key in the above test.
test/jdk/java/util/HashMap/PutNullKey.java line 122:
> 120: }
> 121:
> 122: private static void testCollidingHash() {
Suggestion:
private static void testCollidingHash(IntFunction<?> function) {
And the function can be `CollidingHash::new` and `CollidingHashValue::new`
test/jdk/java/util/HashMap/ReplaceExisting.java line 63:
> 61: }
> 62:
> 63: private static HashMap<VClass,Integer> prepTupleHashMap() {
I doubt the value of these new tests here because `Integer` is already a value
class when preview features are enabled in the current runtime.
test/jdk/java/util/HashMap/SetValue.java line 46:
> 44: }
> 45:
> 46: private static void testStringValue() {
Recommended parameterizing this method as
Suggestion:
private static void test(Object key, Object oldValue, Object newValue) {
and remove the static final fields from this test class.
test/jdk/java/util/HashMap/ToArray.java line 148:
> 146: checkToArray("One", new String[]{"foo"}, set, !ordered);
> 147: set.add("bar");
> 148: checkToArray("Two", new String[]{"foo", "bar"}, set, !ordered);
Same comments, I think you can just add VClass version of this without
shadowing the Long ones because Long is a value class with preview on. So you
can remove checkVClassSet but we might keep checkVClassMap.
test/jdk/java/util/HashMap/TreeBinAssert.java line 179:
> 177:
> 178: @AsValueClass
> 179: static class KeyVClass implements Comparable<KeyVClass> {
Why can't we just `@AsValueClass` the `Key` class itself? When we don't use the
value class plugin, we are still testing the identity versions.
-------------
PR Review: https://git.openjdk.org/jdk/pull/32201#pullrequestreview-4858713787
PR Review Comment: https://git.openjdk.org/jdk/pull/32201#discussion_r3715977020
PR Review Comment: https://git.openjdk.org/jdk/pull/32201#discussion_r3716716986
PR Review Comment: https://git.openjdk.org/jdk/pull/32201#discussion_r3716721919
PR Review Comment: https://git.openjdk.org/jdk/pull/32201#discussion_r3716728848
PR Review Comment: https://git.openjdk.org/jdk/pull/32201#discussion_r3716740798
PR Review Comment: https://git.openjdk.org/jdk/pull/32201#discussion_r3716750552
