RFR: 8266571: Sequenced Collections [v8]

[Stuart Marks]

On Thu, 20 Apr 2023 12:09:52 GMT, Nir Lisker <nlisker at openjdk.org> wrote:

>> Stuart Marks has updated the pull request incrementally with four additional commits since the last revision:
>> 
>>  - Add missing @throws and @since tags.
>>  - Convert code samples to snippets.
>>  - Various editorial changes.
>>  - Fix up toArray(T[]) on reverse-ordered views.
>
> src/java.base/share/classes/java/util/SequencedMap.java line 127:
> 
>> 125:      * Returns a reverse-ordered <a href="Collection.html#view">view</a> of this map.
>> 126:      * The encounter order of elements in the returned view is the inverse of the encounter
>> 127:      * order of elements in this map. The reverse ordering affects all order-sensitive operations,
> 
> Although I think this is clear as-is, you defined *encounter order* in a map with respect to mappings, not elements, and that is also the language used in the methods here, so you might want to use this language here too.

Yes, should be mappings, not elements.

> 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.

> src/java.base/share/classes/java/util/SequencedMap.java line 263:
> 
>> 261:      *
>> 262:      * @implSpec
>> 263:      * The implementation of this method in this class returns a {@code SequencedSet}
> 
> Some of the methods here use "of this method" and some skip it, noting in case you want to streamline it.

Yes my previous standard wording included "of this method" but I forgot to take it out here. Incidentally @jddarcy convinced me to change "class" to "interface" here and in all other interface default methods, so I'll do that as well.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/7387#discussion_r1174193560
PR Review Comment: https://git.openjdk.org/jdk/pull/7387#discussion_r1174194746
PR Review Comment: https://git.openjdk.org/jdk/pull/7387#discussion_r1174195143