On Fri, 2 Jun 2023 01:12:32 GMT, Stuart Marks <sma...@openjdk.org> wrote:
> Create and use new NullableKeyValueHolder class to accommodate map entries
> whose key or value might be null.
Changes look good.
An observation:
`TreeMap` implements `SequencedMap`, and I see that its `firstEntry()` and
related methods use `AbstractMap.SimpleImmutableEntry` (via `exportEntry()`),
despite it being serializable. However, this is long-standing code (from 1.6,
perhaps?), and `TreeMap` is itself serializable. So, leaving it as is seems the
right thing to do.
src/java.base/share/classes/jdk/internal/util/NullableKeyValueHolder.java line
65:
> 63: * Map.of might still need to reject nulls, and so would Map.ofEntries)
> but allowing
> 64: * a Map.Entry itself to contain nulls seems beneficial in general. If
> this is done,
> 65: * merging KeyValueHolder and NullableKeyValueHolder should be
> reconsidered.
I think having this background and explanation in the docs for this internal
class is fine.
IMO, this information would also be useful to have in the bug report.
-------------
Marked as reviewed by bchristi (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/14278#pullrequestreview-1463441861
PR Review Comment: https://git.openjdk.org/jdk/pull/14278#discussion_r1218591121
