RFR: 8266571: Sequenced Collections [v2]

[Rémi Forax]

On Sat, 25 Mar 2023 03:54:23 GMT, Stuart Marks <smarks at openjdk.org> wrote:

>> PR for Sequenced Collections implementation.
>
> Stuart Marks has updated the pull request incrementally with two additional commits since the last revision:
> 
>  - More specification tweaks.
>  - Add simple overrides to ArrayList.

src/java.base/share/classes/java/util/LinkedHashSet.java line 297:

> 295:      */
> 296:     public SequencedSet<E> reversed() {
> 297:         class ReverseLinkedHashSetView extends AbstractSet<E> implements SequencedSet<E> {

This class should be declared `static` (and private) which means it should not be declared inside reversed.

src/java.base/share/classes/java/util/LinkedHashSet.java line 313:

> 311:                 boolean present = LinkedHashSet.this.contains(e);
> 312:                 LinkedHashSet.this.addFirst(e);
> 313:                 return ! present;

why there is a space between '!' and 'present' given that you have removed this space in another change above ?

src/java.base/share/classes/java/util/LinkedList.java line 1293:

> 1291:     @SuppressWarnings("serial")
> 1292:     static class ReverseOrderLinkedListView<E> extends LinkedList<E> implements java.io.Externalizable {
> 1293:         final LinkedList<E> list;

Using 3 different fields feel ugly given it seems you only need one.
Why do you not use the casting strategy you are using for the other views ?

src/java.base/share/classes/java/util/List.java line 802:

> 800:      */
> 801:     default E getFirst() {
> 802:         if (this.isEmpty())

weirdly, sometimes you use braces around the ''if and sometimes you don't ?

src/java.base/share/classes/java/util/ReverseOrderListView.java line 60:

> 58:     }
> 59: 
> 60:     ReverseOrderListView(List<E> list, boolean modifiable) {

should be 'private' to force users to use 'of'

src/java.base/share/classes/java/util/ReverseOrderListView.java line 191:

> 189:         if (o == this)
> 190:             return true;
> 191:         if (!(o instanceof List))

should be `if (!(o instanceof List<?> list))`

src/java.base/share/classes/java/util/ReverseOrderListView.java line 209:

> 207:         int hashCode = 1;
> 208:         for (E e : this)
> 209:             hashCode = 31*hashCode + (e==null ? 0 : e.hashCode());

`31 * hashCode` instead of `31*hashCode`

src/java.base/share/classes/java/util/ReverseOrderSortedMapView.java line 44:

> 42:     public static <K, V> SortedMap<K, V> of(SortedMap<K, V> map) {
> 43:         if (map instanceof ReverseOrderSortedMapView) {
> 44:             return ((ReverseOrderSortedMapView<K, V>)map).base;

use `map instanceof ReverseOrderSortedMapView<K, V> view` instead to remove the ugly cast below

src/java.base/share/classes/java/util/ReverseOrderSortedMapView.java line 215:

> 213:     static <K, V> Iterator<V> descendingValueIterator(SortedMap<K, V> map) {
> 214:         return new Iterator<>() {
> 215:             Iterator<K> keyIterator = descendingKeyIterator(map);

`private final` or better declare it above as a local variable and capture it inside the anonymous class

src/java.base/share/classes/java/util/ReverseOrderSortedMapView.java line 329:

> 327:                 K prevKey = null;
> 328:                 boolean dead = false;
> 329:                 Iterator<Entry<K, V>> it = descendingEntryIterator(base);

see above about declare it as a local variable, all other fields should be `private`

src/java.base/share/classes/java/util/ReverseOrderSortedSetView.java line 61:

> 59:             return true;
> 60: 
> 61:         if (!(o instanceof Set))

use `if (!(o instanceof Set<?> set))` to avoid the cast below

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

PR Review Comment: https://git.openjdk.org/jdk/pull/7387#discussion_r1148321554
PR Review Comment: https://git.openjdk.org/jdk/pull/7387#discussion_r1148321635
PR Review Comment: https://git.openjdk.org/jdk/pull/7387#discussion_r1148321852
PR Review Comment: https://git.openjdk.org/jdk/pull/7387#discussion_r1148322049
PR Review Comment: https://git.openjdk.org/jdk/pull/7387#discussion_r1148322292
PR Review Comment: https://git.openjdk.org/jdk/pull/7387#discussion_r1148322490
PR Review Comment: https://git.openjdk.org/jdk/pull/7387#discussion_r1148322511
PR Review Comment: https://git.openjdk.org/jdk/pull/7387#discussion_r1148322728
PR Review Comment: https://git.openjdk.org/jdk/pull/7387#discussion_r1148322895
PR Review Comment: https://git.openjdk.org/jdk/pull/7387#discussion_r1148323316
PR Review Comment: https://git.openjdk.org/jdk/pull/7387#discussion_r1148323602