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

[Stuart Marks]

Hi Jaikiran,

Terribly sorry about this. I completely missed the review request you posted on 
this a couple weeks ago now.

Comments below.

> Returns a fixed-size list backed by the specified array.

OK. Same as the original. :-)

> 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.

> 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.

> 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:

>      * <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}.

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.

> 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.

> 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.

The latter statement is unfortunately true; the various collections 
implementations aren't consistent as to whether they will fail if a disallowed 
method is called or whether such a call would actually modify the list. For 
example, appending an empty list to a Collections.unmodifiableList() will throw 
UOE, but this is a no-op for a Collections.singletonList(). Note however that 
there is only one implementation of Arrays.asList() so the "certain 
implementations" discussion is misplaced. There's already some discussion of 
this issue in the Collection class doc, so this isn't needed here.

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

  - restore the "Three Stooges" example (beginning with "This method also 
provides...")

  - 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.

>      * @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.

**

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.

Thanks,

s'marks







On 9/10/18 12:31 AM, Jaikiran Pai wrote:
> Any other reviews? I'm not a committer, so I'll also need someone to
> help sponsor this patch.
> 
> -Jaikiran
> 
> 
> On 06/09/18 7:39 PM, Jaikiran Pai wrote:
>> On 06/09/18 1:24 PM, Bernd Eckenfels wrote:
>>> Yes you are right @apinote is aproperiate section (was confusing it with implnote).
>>>
>>> Still think a ‚literal specified list‘ is no longer a good (as in canonical) usecase for that method.
>>>
>>> I used it in the past often to get a List for using toString() on it, but I guess even for that case List.of can be used instead now.
>>>
>>> So @see List#of and let the reader figure out when to use them?
>> That sounds good to me. I've now updated it to reflect this change and
>> the javadoc now looks as below. I've also attached the new updated patch.
>>
>>      /**
>>       * Returns a fixed-size list backed by the specified array. The passed
>>       * array is held as a reference in the returned list. 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. The returned list is serializable and
>>       * implements {@link RandomAccess}.
>>       *
>>       * <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.
>>       *
>>       * <p>This method acts as bridge between array-based and
>> collection-based
>>       * APIs, in combination with {@link Collection#toArray}.
>>       *
>>       * @apiNote
>>       * 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.
>>       *
>>       * @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()
>>       */
>>
>>
>>
>> -Jaikiran
>>
>