[PATCH] JDK-7033681 - Improve the documentation of Arrays.asList

[Jaikiran Pai]

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 Jaikiran Pai <jaikiran.pai at gmail.com>
# Date 1535208403 -19800
#      Sat Aug 25 20:16:43 2018 +0530
# Node ID fbb71a7edc1aeeb6065e309a3efa67bc7ead6a3c
# Parent  6c394ed56b07d453226e064bc011528cd89b7d69
JDK-7033681 Improve the javadoc of Arrays#toList()

diff --git a/src/java.base/share/classes/java/util/Arrays.java b/src/java.base/share/classes/java/util/Arrays.java
--- a/src/java.base/share/classes/java/util/Arrays.java
+++ b/src/java.base/share/classes/java/util/Arrays.java
@@ -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,11 +4289,25 @@
     // 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 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:
@@ -4300,9 +4315,14 @@
      *     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}
      */
     @SafeVarargs
     @SuppressWarnings("varargs")