On Sat, 25 Mar 2023 03:54:23 GMT, Stuart Marks <sma...@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
