Hi Jaikiran,
Thanks for doing the updates and sending the patch.
Replies to your comments follow.
While adding this, I realized that the current javadoc doesn't mention
anything about the behaviour of this method when a null array is passed
to it. The implementation currently throws a NullPointerException. So I
went ahead and updated the javadoc to include:
* @throws NullPointerException if the passed array is {@code null}
Strictly speaking this isn't necessary, as the class doc has the following
statement:
* <p>The methods in this class all throw a {@code NullPointerException},
* if the specified array reference is null, except where noted.
However, since Arrays.asList is quite heavily used, and it's something of an
outlier among the other Arrays methods, I think it's OK to add the @throws line.
(There are a few other methods in this class that also have such @throws lines.)
- add a note emphasizing that the returned list is NOT unmodifiable.
it's a common mistake to assume that it is. I'd also put in a
reference to List.html#unmodifiable here in case people do want an
unmodifiable list.
Done. I looked up the javadoc style guide[1] (is that still relevant,
especially for contributions to the JDK itself?) and javadoc of some
"recent" files within the JDK code, to see what's the current way to
emphasize certain parts of the comments. I couldn't find anything
conclusive, so I decided to use "<em>". Let me know if there's some
other preferred way.
[1]
http://www.oracle.com/technetwork/java/javase/documentation/index-137868.html#styleguide
The JDK has a mixture of "<em>" vs "<i>" markup. Since you want to provide
emphasis, "<em>" seems sensible. (Note, I've modified this further; see below.)
The javadoc style guide is quite old and many parts are out of date, but many
parts are still valid. For example, it's still a strong convention that a method
doc's initial sentence be a verb phrase (not a complete sentence) in the
indicative (not imperative) mood. For example, the summary sentence of this
method's doc is:
Returns a fixed-size list backed by the specified array.
and it is not
Return a fixed-size list backed by the specified array.
and it is also not
This method returns a fixed-size list backed by the specified array.
However, the javadoc style guide mentions use of <code>...</code> markup, which
has superseded by the javadoc {@code ...} inline tag. I think that tag was
introduced more recently than the javadoc style guide was last updated.
**
The patch you sent is pretty good as it stands, but there were a couple minor
things that I think could still be improved. In the interest of time, instead of
asking you to make the changes, I went ahead and modified the patch myself. The
modifications are as follows:
- * <p>The returned list can be changed only in certain ways. Operations
- * that would change the size of the returned list leave the list unchanged
- * and throw {@link UnsupportedOperationException}.
The first sentence didn't seem right to me, as it's quite vague. After some
thought it finally occurred to me that what's necessary is a statement about the
optional methods of the Collection interface. I've edited this paragraph thus:
+ * <p>The returned list implements the optional Collection methods, except
those
+ * that would change the size of the returned list. Those methods leave
+ * the list unchanged and throw {@link UnsupportedOperationException}.
In the last paragraph I inverted the sense of "not unmodifiable" and added a
link to the Collections.unmodifiableList method.
+ * <p>The list returned by this method <em>is modifiable.</em>
+ * To create an unmodifiable list, see
+ * {@link Collections#unmodifiableList Collections.unmodifiableList}
+ * or <a href="List.html#unmodifiable">Unmodifiable Lists</a>.
I've changed the <pre></pre> code samples to use <pre>{@code ...}</pre>. This
allows use of < and > instead of HTML entities < and >.
Finally, I had to change the changeset metadata to conform to the OpenJDK rules.
Specifically, the changeset author is required to be an OpenJDK user name. Since
you don't have one, I've listed your email address in the Contributed-by line of
the changeset comment.
I've attached the modified patch. If you're ok with it, I'll proceed with filing
a CSR.
Thanks,
s'marks
On 9/15/18 1:49 AM, Jaikiran Pai wrote:
Hello Stuart,
Thank you very much for the detailed review. I have attached an updated
patch which incorporates the suggested changes. The complete javadoc
text is included inline here and a few comments are inline in the rest
of this mail:
/**
* Returns a fixed-size list backed by the specified array. Changes
made to
* the array will be visible in the returned list, and changes made
to the
* list will be visible in the array. The returned list is
* {@link Serializable} and implements {@link RandomAccess}.
*
* <p>The returned list can be changed only in certain ways. Operations
* that would change the size of the returned list leave the list
unchanged
* and throw {@link UnsupportedOperationException}.
*
* @apiNote
* This method acts as bridge between array-based and collection-based
* APIs, in combination with {@link Collection#toArray}.
*
* <p>This method provides a way to wrap an existing array:
* <pre>
* Integer[] numbers = ...
* ...
* List<Integer> values = Arrays.asList(numbers);
* </pre>
*
* <p>This method also provides a convenient way to create a fixed-size
* list initialized to contain several elements:
* <pre>
* List<String> stooges = Arrays.asList("Larry", "Moe",
"Curly");
* </pre>
*
* <p>The list returned by this method is <em>not</em>
* <a href="{@docRoot}/java.base/java/util/List.html#unmodifiable">
* unmodifiable</a>
*
* @param <T> the class of the objects in the array
* @param a the array by which the list will be backed
* @return a list view of the specified array
* @throws NullPointerException if the passed array is {@code null}
*/
On 11/09/18 7:54 AM, Stuart Marks wrote:
The passed array is held as a reference in the returned list.
While this is true, this is really a statement about implementation.
For the specification, we want to focus on assertions about the
behavior. Please remove.
Done.
Any subsequent changes made to the array contents will be visible in the
returned list. Similarly any changes that happen in the returned list
will
also be visible in the array.
This is an improvement on the original "(Changes to the returned list
"write through" to the array)" which doesn't describe the behavior
precisely enough nor does it discuss the "bi-directional" nature of
visibility of the changes. The proposed text is a bit verbose,
however; I'd suggest the following:
Changes made to the array will be visible in the returned list, and
changes made to the list will be visible in the array.
Done. I agree the my previous version in the patch was verbose and what
you suggest looks better.
The returned list is serializable and implements {@link RandomAccess}.
(This was from the original text.) It's kind of odd that RandomAccess
is a link but Serializable is not. I'd suggest making Serializable a
link as follows:
Done.
* <p>The returned list can be changed only in certain ways.
Operations
* like {@code add}, {@code remove}, {@code clear} and other
such, that
* change the size of the list aren't allowed. Operations like
* {@code replaceAll}, {@code set}, that change the elements in
the list
* are permitted.
This text would be fine for something like a tutorial, but it's not
precise enough in that it describes behavior by example ("operations
like...") and it doesn't specify the behavior if a disallowed
operation is performed. I'd suggest this instead:
Operations that would change the size of the returned list leave the
list unchanged and throw {@link UnsupportedOperationException}.
Done.
Listing the exact methods that can change the size is unwise, as it
can change over time and the list will become out of date. There are
also a lot of obscure ways to change the size of a list besides
methods on the list, e.g. via an iterator's remove(), a sublist's
size-change method, removeIf(), etc. and it's not worth trying to
chase them all down.
Agreed. In fact that was the reason why I originally used "operations
like...", but your suggested change above, to this text, I believe
covers all these relevant APIs.
This method acts as bridge between array-based and collection-based
APIs, in
combination with {@link Collection#toArray}.
This (from the original text) should actually be part of the @apiNote.
However, it was written before the @apiNote concept existed. So, start
the @apiNote before this text.
Done.
The returned list throws a {@link UnsupportedOperationException} for
operations that aren't permitted. Certain implementations of the
returned
list might choose to throw the exception only if the call to such
methods
results in an actual change, whereas certain other implementations
may always
throw the exception when such methods are called.
This UOE statement now redundant with the above.
This has now been removed in favour of the suggested change.
I do think it's reasonable to have examples here. Interestingly, the
original (pre-varargs) purpose of Arrays.asList() was to wrap an
existing array in a list. With varargs, this method is probably now
more often used to create a list from arguments. Both uses are valid,
but the first is now less obvious and so I think deserves a new
example. The latter remains valid even with List.of() and friends,
since those methods make a copy of the array that might not be
necessary in all cases.
Here's what I'd suggest:
- add a simple (~2 line) example showing wrapping of an existing array
in a list
Done. Added a new example to show this:
* <p>This method provides a way to wrap an existing array:
* <pre>
* Integer[] numbers = ...
* ...
* List<Integer> values = Arrays.asList(numbers);
* </pre>
While adding this, I realized that the current javadoc doesn't mention
anything about the behaviour of this method when a null array is passed
to it. The implementation currently throws a NullPointerException. So I
went ahead and updated the javadoc to include:
* @throws NullPointerException if the passed array is {@code null}
- restore the "Three Stooges" example (beginning with "This method
also provides...")
Done.
- add a note emphasizing that the returned list is NOT unmodifiable.
it's a common mistake to assume that it is. I'd also put in a
reference to List.html#unmodifiable here in case people do want an
unmodifiable list.
Done. I looked up the javadoc style guide[1] (is that still relevant,
especially for contributions to the JDK itself?) and javadoc of some
"recent" files within the JDK code, to see what's the current way to
emphasize certain parts of the comments. I couldn't find anything
conclusive, so I decided to use "<em>". Let me know if there's some
other preferred way.
* @param <T> the class of the objects in the array
* @param a the array by which the list will be backed
* @return a list view of the specified array
* @see List#of()
OK. We probably don't need the @see for List.of if there's a reference
to the "unmodifiable lists" section above.
Done. "@see List#of()" is now removed in this updated version. Bernd,
let us know if you agree that this update to include reference to the
List.html#unmodifiable covers the use case for which we introduced the
List#of() reference.
I can sponsor this change for you. Since we're changing testable
assertions, this will also require a CSR request. I'll take care of
that for you too.
Thank you :)
[1]
http://www.oracle.com/technetwork/java/javase/documentation/index-137868.html#styleguide
-Jaikiran
# HG changeset patch
# User smarks
# Date 1535208403 -19800
# Sat Aug 25 20:16:43 2018 +0530
# Node ID fbb71a7edc1aeeb6065e309a3efa67bc7ead6a3c
# Parent 9bf5205655ee3a8199abda638ad685b76a43a6fc
7033681: Arrays.asList methods needs better documentation
Reviewed-by: smarks
Contributed-by: jai.forums2...@gmail.com
diff -r 9bf5205655ee src/java.base/share/classes/java/util/Arrays.java
--- a/src/java.base/share/classes/java/util/Arrays.java Fri Sep 14 12:10:28
2018 -0400
+++ b/src/java.base/share/classes/java/util/Arrays.java Wed Sep 19 10:31:21
2018 -0700
@@ -28,6 +28,7 @@
import jdk.internal.HotSpotIntrinsicCandidate;
import jdk.internal.util.ArraysSupport;
+import java.io.Serializable;
import java.lang.reflect.Array;
import java.util.concurrent.ForkJoinPool;
import java.util.function.BinaryOperator;
@@ -4288,21 +4289,41 @@
// Misc
/**
- * Returns a fixed-size list backed by the specified array. (Changes to
- * the returned list "write through" to the array.) This method acts
- * as bridge between array-based and collection-based APIs, in
- * combination with {@link Collection#toArray}. The returned list is
- * serializable and implements {@link RandomAccess}.
+ * Returns a fixed-size list backed by the specified array. Changes made to
+ * the array will be visible in the returned list, and changes made to the
+ * list will be visible in the array. The returned list is
+ * {@link Serializable} and implements {@link RandomAccess}.
+ *
+ * <p>The returned list implements the optional Collection methods, except
those
+ * that would change the size of the returned list. Those methods leave
+ * the list unchanged and throw {@link UnsupportedOperationException}.
+ *
+ * @apiNote
+ * This method acts as bridge between array-based and collection-based
+ * APIs, in combination with {@link Collection#toArray}.
+ *
+ * <p>This method provides a way to wrap an existing array:
+ * <pre>{@code
+ * Integer[] numbers = ...
+ * ...
+ * List<Integer> values = Arrays.asList(numbers);
+ * }</pre>
*
* <p>This method also provides a convenient way to create a fixed-size
* list initialized to contain several elements:
- * <pre>
- * List<String> stooges = Arrays.asList("Larry", "Moe", "Curly");
- * </pre>
+ * <pre>{@code
+ * List<String> stooges = Arrays.asList("Larry", "Moe", "Curly");
+ * }</pre>
+ *
+ * <p>The list returned by this method <em>is modifiable.</em>
+ * To create an unmodifiable list, see
+ * {@link Collections#unmodifiableList Collections.unmodifiableList}
+ * or <a href="List.html#unmodifiable">Unmodifiable Lists</a>.
*
* @param <T> the class of the objects in the array
* @param a the array by which the list will be backed
* @return a list view of the specified array
+ * @throws NullPointerException if the specified array is {@code null}
*/
@SafeVarargs
@SuppressWarnings("varargs")
