[PATCH] JDK-7033681 - Improve the documentation of Arrays.asList
Stuart Marks
stuart.marks at oracle.com
Wed Sep 19 17:36:03 UTC 2018
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
>
-------------- next part --------------
# 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.forums2013 at 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")
More information about the core-libs-dev
mailing list