Hello Bernd, Thank you for the review and sorry about the delayed response. Comments inline. On 29/08/18 4:26 PM, Bernd Eckenfels wrote:

Hello,

Not an Reviewer But just wanted to give a short Feedback: I like the new Version it is really helpful. 

However I wonder if the usage example should be outside of the apinote.

+ * @apiNote + * 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> * My limited understanding of the @apiNote is that it is supposed to be used for additional details of the API and/or its usage examples and the javadoc itself (outside of the @apiNote) should be a specification of the API. In this case, I moved that section to @apiNote since that part appears to mention how/when to use that API in. Having said that, I can move it out of @apiNote and let it stay the way it previously was, if you and others feel that's the way to go.

Given the existence of List.of() I wonder if you either mention it as a alternative to the example (with slightly different semantic) or just remove the sample completely?

I'm not too sure mentioning List.of() construct here will be useful, but I do see why you mention that. I think the existing example does seem like a useful usage example, irrespective of whether or not we decide to have it in or outside of an @apiNote. -Jaikiran

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

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

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

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

-Jaikiran

On 9/19/18 7:46 PM, Jaikiran Pai wrote:

Thank you. More of a FYI and if it matters from a process point of view - in a couple of my earlier contributions, the sponsors have used the "Contributed-by" line to be "Jaikiran Pai <jaikiran.pai@gmail.com>" like here http://hg.openjdk.java.net/jdk/jdk/rev/6c394ed56b07. I don't have any specific preference on which one is used.

I'm happy to change the Contributed-by line to anything you want. After all this represents YOU and it's recorded in the changeset history. It's probably good to be consistent. Unfortunately I see the following changesets in the history right now: changeset: 51755:6c394ed56b07 user: xuelei date: Fri Sep 14 20:30:28 2018 -0700 files: src/java.base/share/classes/javax/net/ssl/X509ExtendedKeyManager.java description: 8210785: Trivial typo fix in X509ExtendedKeyManager javadoc Reviewed-by: xuelei Contributed-by: Jaikiran Pai <jaikiran.pai@gmail.com> changeset: 49926:b7c2996d690b user: chegar date: Mon Apr 30 16:13:30 2018 +0100 files: src/java.base/share/classes/java/net/InetAddress.java test/jdk/java/net/InetAddress/GetLoopbackAddress.java description: 8201545: InetAddress.getByName/getAllByName should clarify empty String behavior Reviewed-by: chegar Contributed-by: Jaikiran Pai <jai.forums2013@gmail.com> Since you mentioned it I'll proceed with "Jaikiran Pai <jaikiran.pai@gmail.com>" since that looks a bit more "official". But do let me know if you have a different preference.

I re-read the whole javadoc contained in your modified patch and it all looks good to me. Thank you very much for the help in sponsoring, reviewing and providing detailed inputs during the review.

You're welcome. OK, I'll proceed with the CSR. s'marks

On 9/20/18 7:01 PM, Jaikiran Pai wrote:

...

Since you mentioned it I'll proceed with "Jaikiran Pai <jaikiran.pai@gmail.com>" since that looks a bit more "official".

Sounds fine to me and yes it indeed is a bit more official one. Thank you.

OK, CSR request posted: https://bugs.openjdk.java.net/browse/JDK-8210991 Can I get a reviewer for this please? (Sorry Jaikiran, I don't think you can review it, as you need to have an OpenJDK user name and a JIRA account. But I'd encourage you to take a look to see what I did.) s'marks

Hello Stuart, Thank you very much for your help in sponsoring this as well as the detailed reviews and suggestions. -Jaikiran On Thursday, September 27, 2018, Stuart Marks <stuart.marks@oracle.com> wrote:

Hi, I just wanted to mention that I pushed the changeset for this bug:

http://hg.openjdk.java.net/jdk/jdk/rev/2ee7e1b7ba66

Thanks for your contribution to OpenJDK!

s'marks

Hi Jon, Yes, definitely, given the scope of the current proposed changes, I intend to file a CSR for this changeset on Jaikiran's behalf. s'marks On 9/19/18 10:42 AM, Jonathan Gibbons wrote:

Jaikiran,

Referring back to the original email discussion, changes like this may require a CSR to be filed.

From Stuart Marks: http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-August/054894.html

...

Whether this requires a CSR depends on whether any normative text of the specification is changed. A simple clarification ("API note") can be added without a CSR. As the wording stands now, however, it seems like there is a mixture of normative and informative statements in the text; these should be teased apart and the informative statements placed into an API note. In addition, the normative text could probably be reworded to be more clear. (See my comments in the bug.) Such changes would probably need a CSR, even though they wouldn't actually change the intent of the specification.

-- Jon

On 08/20/2018 05:26 AM, Jaikiran Pai wrote:

...

Hello everyone,

I'm requesting a review of a documentation change which was discussed in a recent thread[1][2]. Here's an initial proposed draft, for a better documentation of Arrays.asList method:

     /**       * 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       * 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 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       */      @SafeVarargs      @SuppressWarnings("varargs")      public static <T> List<T> asList(T... a)

I've edited some of the existing documentation of that method, moved some existing parts into @apiNote and added additional parts both to the spec as well as the @apiNote. For a complete reference of what's changed, I've also attached a patch of this change.

P.S: Is there a specific (make) target that I can run to make sure changes like this one haven't caused any javadoc generation issues? I typically run just "make" and did it this time too, but I'm not sure it parses and generates the javadocs (couldn't find it in the generated exploded build image).

[1] http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-August/054894.html

[2] https://bugs.openjdk.java.net/browse/JDK-7033681

-Jaikiran