On Fri, 21 Apr 2023 22:14:58 GMT, Stuart Marks <sma...@openjdk.org> wrote:
>> src/java.base/share/classes/java/util/SequencedMap.java line 152:
>>
>>> 150: var it = entrySet().iterator();
>>> 151: return it.hasNext() ? Map.Entry.copyOf(it.next()) : null;
>>> 152: }
>>
>> Would is be better to first check `Map.isEmpty()` and only then obtain the
>> iterator? That would eliminate also the `hasNext` check.
>>
>> default Map.Entry<K,V> firstEntry() {
>> return isEmpty() ? null : Map.Entry.copyOf(entrySet().iterator().next());
>> }
>>
>>
>> Same question for the other methods.
>
> Might not work if this map is concurrent. If isEmpty() returns false but then
> the map is emptied before the subsequent calls, next() might throw NSEE. The
> concurrent maps' iterators make sure to cache the element if hasNext() has
> returned true.
Then shouldn't `ConcurrentNavigableMap`, the only `ConcurrentMap` and
`SequencedMap` supertype (there is no `ConcurrentSortedMap`), override its
superinterface implementation with this thread-safe one?
I think that the question boils down to: should `SequencedMap` take into
account the needs of its subtypes and give a conservative default
implementation, or should it give a "minimal" implementation and let the
subtypes override it according to their needs? I would choose the latter, both
because `SequencedMap` specifies no guarantees on thread-safety (atomicity and
synchronization), and because `Map`'s default implementation behave this way -
`compute`, `merge`... tell concurrent implementations how to override them.
Now if someone implements a `SequencedMap` that is also a `ConcurrentMap`, but
is not a `NavigableMap`, say `ConcurrentLinkedHashMap`, they will override the
`SequencedMap` defaults like they would the `Map` defaults.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/7387#discussion_r1174327323
