RFR: 8266571: Sequenced Collections [v7]
Nir Lisker
nlisker at openjdk.org
Wed Apr 19 14:45:46 UTC 2023
On Wed, 19 Apr 2023 03:37:05 GMT, Stuart Marks <smarks at openjdk.org> wrote:
>> PR for Sequenced Collections implementation.
>
> Stuart Marks has updated the pull request incrementally with three additional commits since the last revision:
>
> - Remove unnecessary 'final' from a couple places.
> - Clarify ordering of Collection.addAll and Map.putAll; add links to
> encounter order.
> - Make constructors private for several reverse-ordered views.
I reviewed some of the public documentation and left some comments and thoughts. As a general remark, the documentation could use [Code Snippets](https://openjdk.org/jeps/413) to avoid the `<pre>{@code` pattern.
src/java.base/share/classes/java/util/Collection.java line 511:
> 509: * nonempty.) If the specified collection has a defined
> 510: * <a href="SequencedCollection.html#encounter">encounter order</a>,
> 511: * processing of its elements generally occurs in that order.
Shouldn't `remove` also contain such a note? From its docs:
"More formally, removes an element e such that `Objects.equals(o, e)`, if this collection contains one or more such elements." Can add something like "If the specified collection has a defined *encounter order*, generally removes the first encountered element in that order".
src/java.base/share/classes/java/util/Collections.java line 374:
> 372: * @apiNote
> 373: * This method mutates the specified list in-place. To obtain a
> 374: * reversed-ordered view of a list without mutating it, use the
The term that has been usually used is "reverse-ordered".
src/java.base/share/classes/java/util/Collections.java line 380:
> 378: * @throws UnsupportedOperationException if the specified list or
> 379: * its list-iterator does not support the {@code set} operation.
> 380: * @see List#reversed List.reversed
Is the `@see` tag really useful if you're already linking the method directly?
src/java.base/share/classes/java/util/Collections.java line 6069:
> 6067: *
> 6068: * @apiNote
> 6069: * This method provides a view that inverts the sense of certain operations,
I think that "inverts the operation of certain methods" is clearer.
src/java.base/share/classes/java/util/Collections.java line 6070:
> 6068: * @apiNote
> 6069: * This method provides a view that inverts the sense of certain operations,
> 6070: * but it doesn't reverse the encounter order. To obtain a reversed-ordered
The term that has been usually used is "reverse-ordered".
src/java.base/share/classes/java/util/Collections.java line 6071:
> 6069: * This method provides a view that inverts the sense of certain operations,
> 6070: * but it doesn't reverse the encounter order. To obtain a reversed-ordered
> 6071: * view, use the {@link Deque#reversed Deque::reversed} method.
In `@link` tags in other places, the display name uses `Class.method` formatting, not `Class::method`. Might want to consolidate this.
src/java.base/share/classes/java/util/List.java line 813:
> 811: *
> 812: * @implSpec
> 813: * If this List is not empty, the implementation in this class returns the result
`{@code List}`
Same in other places.
src/java.base/share/classes/java/util/List.java line 882:
> 880: * @implSpec
> 881: * The implementation in this class returns an instance of a reverse-ordered
> 882: * List that delegates its operations to this List.
`{@code List}`
src/java.base/share/classes/java/util/NavigableSet.java line 361:
> 359: * {@inheritDoc}
> 360: * <p>
> 361: * This method is equivalent to {@link #descendingSet descendingSet}.
Doesn't `{@link #descendingSet}` display as `descendingSet` already? The same pattern appears in other places.
src/java.base/share/classes/java/util/SequencedCollection.java line 1:
> 1: /*
I wonder if there's a point to mention the relation of reversal to the "enhanced for-each" loop. The loop uses the iterator behind the scenes. That means that a reversed collection will iterate backwards in a loop ("enhanced each-for" loop :) ). A backwards iteration can be achieved by
for (E element : collection.reversed()) {...}
For collections that support removal, this construct can also be used to remove elements without the concurrent modification problem of removal during iteration which messes up the indices. See, for example, https://stackoverflow.com/questions/10431981/remove-elements-from-collection-while-iterating.
src/java.base/share/classes/java/util/SequencedCollection.java line 34:
> 32: * from the first element to the last element. Given any two elements, one element is
> 33: * either before (closer to the first element) or after (closer to the last element)
> 34: * the other element.
The last sentence seems a bit clumsy owing to the abundance of parenthesized content and the confusion of "first element" of the collection with the first element of the 2 chosen for the explanation. How about something like:
"Given any two elements, one element is either before the other element (it is closer to the collection's start) or after it (closer to the collection's end)"?
src/java.base/share/classes/java/util/SequencedCollection.java line 49:
> 47: * <p>
> 48: * This interface provides methods to add elements, to retrieve elements, and to remove
> 49: * elements at either end of the collection.
Maybe this phrasing will be easier to read:
"This interface provides methods to add, retrieve, and remove elements..."
src/java.base/share/classes/java/util/SequencedCollection.java line 93:
> 91:
> 92: /**
> 93: * Adds an element as the first element of this collection (optional operation).
"Adds an element as the first element" is a bit confusing. Maybe "at the first position".
src/java.base/share/classes/java/util/SequencedCollection.java line 98:
> 96: *
> 97: * @implSpec
> 98: * The implementation in this class always throws {@code UnsupportedOperationException}.
class -> interface? This appears in other interfaces as well. We tend to code to interfaces with collections, so this distinction seems relevant.
src/java.base/share/classes/java/util/SequencedCollection.java line 102:
> 100: * @param e the element to be added
> 101: * @throws NullPointerException if the specified element is null and this
> 102: * collection does not permit null elements
Not sure if the JDK encourages "null" to be in `@code`. I personally think it's better since it's a literal.
src/java.base/share/classes/java/util/SequencedCollection.java line 123:
> 121: * @throws UnsupportedOperationException if this collection implementation
> 122: * does not support this operation
> 123: */
Same comments as in `addFirst`.
src/java.base/share/classes/java/util/SequencedMap.java line 29:
> 27:
> 28: /**
> 29: * A Map that has a well-defined encounter order, that supports operations at both ends, and
`{@code Map}`
src/java.base/share/classes/java/util/SequencedMap.java line 49:
> 47: * {@link #keySet keySet},
> 48: * {@link #values values}, and
> 49: * {@link #entrySet entrySet} methods are not sequenced <i>types</i>, the elements
These were just linked before, I don't think they should be linked again. `@code` should suffice.
src/java.base/share/classes/java/util/SequencedMap.java line 59:
> 57: * <p>
> 58: * This interface provides methods to add mappings, to retrieve mappings, and to remove
> 59: * mappings at either end of the map's encounter order.
Similar to the other place, I recommend "...to add, retrieve, and remove mappings...".
src/java.base/share/classes/java/util/SequencedMap.java line 74:
> 72: * <pre>{@code
> 73: * var itr = sequencedMap.reversed().entrySet().iterator();
> 74: * }</pre>
To avoid confusion, I would mention the relation between this example and
sequencedMap.sequencedEntrySet().reversed().iterator();
Or, in general, reversing the map and then obtaining a view, compared to obtaining a view and then reversing it.
src/java.base/share/classes/java/util/SequencedMap.java line 85:
> 83: * <p>
> 84: * The {@link Map.Entry} instances obtained by iterating the {@link #entrySet} view, the
> 85: * {@link #sequencedEntrySet} view, and its reverse-ordered view, maintain a connection to the
`#entrySet` and `#sequencedEntrySet` were linked above, no need to link them again.
src/java.base/share/classes/java/util/SequencedMap.java line 88:
> 86: * underlying map. This connection is guaranteed only during the iteration. It is unspecified
> 87: * whether the connection is maintained outside of the iteration. If the underlying map permits
> 88: * it, calling an Entry's {@link Map.Entry#setValue setValue} method will modify the value of the
`{@code Entry}`
src/java.base/share/classes/java/util/SequencedMap.java line 99:
> 97: * return {@link Map.Entry} instances that represent snapshots of mappings as
> 98: * of the time of the call. They do <em>not</em> support mutation of the
> 99: * underlying map via the optional {@link Map.Entry#setValue setValue} method.
Previously linked.
src/java.base/share/classes/java/util/SequencedMap.java line 110:
> 108: * {@code Entry} thus obtained will update a mapping in the underlying map, or whether
> 109: * it will throw an exception, or whether changes to the underlying map are visible in
> 110: * that {@code Entry}.
These 3 paragraphs all talk about `Entry`s' connection to the map. I think that they can be reconciled by explicitly noting each way to get an entry and the conditions that apply on it. In broad strokes:
> `{@link Map.Entry}` instances can be obtained in several ways.
> By iterating the `{@link #entrySet}` view... These entries maintain a connection...
> By calling the `{@link #firstEntry}`... methods. These entries represent snapshots of...
> By other means, such as from methods of views of the map (`sequencedMap.sequencedEntrySet().getFirst();`). These entries entries might or might not be connected...
src/java.base/share/classes/java/util/SequencedSet.java line 30:
> 28: /**
> 29: * A collection that is both a {@link SequencedCollection} and a {@link Set}. As such, it can be
> 30: * thought of as either as a {@code Set} that also has a well-defined encounter order, or as a
"thought of ~~as~~ either"
src/java.base/share/classes/java/util/SequencedSet.java line 30:
> 28: /**
> 29: * A collection that is both a {@link SequencedCollection} and a {@link Set}. As such, it can be
> 30: * thought of as either as a {@code Set} that also has a well-defined encounter order, or as a
Usually you link to `encounter order`, like in `SequencedMap`.
src/java.base/share/classes/java/util/SortedSet.java line 358:
> 356: * @implSpec
> 357: * The implementation in this class returns an instance of a reverse-ordered
> 358: * SortedSet that delegates its operations to this SortedSet.
`{@code SortedSet}`
-------------
PR Review: https://git.openjdk.org/jdk/pull/7387#pullrequestreview-1391301308
PR Review Comment: https://git.openjdk.org/jdk/pull/7387#discussion_r1170836342
PR Review Comment: https://git.openjdk.org/jdk/pull/7387#discussion_r1171047117
PR Review Comment: https://git.openjdk.org/jdk/pull/7387#discussion_r1170840446
PR Review Comment: https://git.openjdk.org/jdk/pull/7387#discussion_r1171044881
PR Review Comment: https://git.openjdk.org/jdk/pull/7387#discussion_r1171047614
PR Review Comment: https://git.openjdk.org/jdk/pull/7387#discussion_r1170998217
PR Review Comment: https://git.openjdk.org/jdk/pull/7387#discussion_r1171150721
PR Review Comment: https://git.openjdk.org/jdk/pull/7387#discussion_r1171246754
PR Review Comment: https://git.openjdk.org/jdk/pull/7387#discussion_r1171264376
PR Review Comment: https://git.openjdk.org/jdk/pull/7387#discussion_r1171093839
PR Review Comment: https://git.openjdk.org/jdk/pull/7387#discussion_r1171018726
PR Review Comment: https://git.openjdk.org/jdk/pull/7387#discussion_r1171010122
PR Review Comment: https://git.openjdk.org/jdk/pull/7387#discussion_r1171098628
PR Review Comment: https://git.openjdk.org/jdk/pull/7387#discussion_r1171121296
PR Review Comment: https://git.openjdk.org/jdk/pull/7387#discussion_r1171123147
PR Review Comment: https://git.openjdk.org/jdk/pull/7387#discussion_r1171125802
PR Review Comment: https://git.openjdk.org/jdk/pull/7387#discussion_r1171271109
PR Review Comment: https://git.openjdk.org/jdk/pull/7387#discussion_r1171282680
PR Review Comment: https://git.openjdk.org/jdk/pull/7387#discussion_r1171280805
PR Review Comment: https://git.openjdk.org/jdk/pull/7387#discussion_r1171293778
PR Review Comment: https://git.openjdk.org/jdk/pull/7387#discussion_r1171296055
PR Review Comment: https://git.openjdk.org/jdk/pull/7387#discussion_r1171298023
PR Review Comment: https://git.openjdk.org/jdk/pull/7387#discussion_r1171299868
PR Review Comment: https://git.openjdk.org/jdk/pull/7387#discussion_r1171417481
PR Review Comment: https://git.openjdk.org/jdk/pull/7387#discussion_r1171267859
PR Review Comment: https://git.openjdk.org/jdk/pull/7387#discussion_r1171273471
PR Review Comment: https://git.openjdk.org/jdk/pull/7387#discussion_r1171246074
More information about the core-libs-dev
mailing list