RFR [16] 8251550: Clean up jdk.javadoc and the related parts of jdk.compiler

Mon Aug 17 12:30:47 UTC 2020

Thanks for having a good look at it, Jon. My replies are inline.

> On 14 Aug 2020, at 17:49, Jonathan Gibbons <jonathan.gibbons at oracle.com> wrote:
> 
> Good cleanup.
> 
> Some systemic changes needed. Details below.
> 
> -- Jon
> 
> On 8/13/20 11:48 AM, Pavel Rappo wrote:
>> Hello,
>> 
>> Please review the below change for 
>> https://bugs.openjdk.java.net/browse/JDK-8251550
>> 
>> 
>>   
>> http://cr.openjdk.java.net/~prappo/8251550/webrev.00/
>> 
>> 
>> This represents squashed commits that have accumulated in my git branch. Since the changeset has started to look dangerously big, I decided not to wait until we finish the migration to Git and GitHub, and push it sooner. The less the others have to merge, the better.
>> 
>> -Pavel
>> 
>> 
> 
> 
> Some of the doctree changes contain this sequence:
> 
>     <p>
>     <pre>
> 
> That will trigger a doccheck warning for a redundant <p> tag.

Fixed.

> Since you're improving the doc comments for the DocTree nodes, it would be nice to use <i>...</i> or (better) <var>,,,</var> around the variables in the templates.  For example:
> 
>   34  * <pre>
>   35  *    @author <var>name-text</var>
>   36  * </pre>
> 
>   37  *

Understood. However, for now I would prefer consistency (with the interfaces of the com.sun.source.tree package) over further improvement.

> DocRootTree:
> 
> I think you need {@literal ...} on line 33.

Why? The doc comment uses an HTML entity, not a naked "@" symbol:

    * <p>
    * <pre>
    *    {@docroot}
    * </pre>

Are you looking at the actual *diff* rather than at a *view* of the webrev? Webrev is infamous for producing a certain type of illusions.

> DocTypeTree:
> As an enhancement, given that the tools only support HTML 5 these days, It might be helpful to give the specific text for an HTML 5 document
> 
>   32  * <pre>
> 
>   33  *    <!doctype text>
> 
>   34  * </pre>
>     + * For HTML5 documents, the correct form is  {@code <!doctype html>}.

Added the proposed line.

> EntityTree:
> Maybe remove the "misleading" spaces in the examples?

Removed the spaces.

> General:
> 
> Thinking aloud, for a future update when we have the `{@spec}` tag. In the introductory sentence for each tag, replace the use of `{@code}` by `{@spec}` linking to the right place in the doc-comment tag specification. I guess for many, it's not necessary, but the idea came to mind reading IndexTree, where more information may be helpful. That being said, additional info belongs in the tag spec, not the tree node spec.

Agreed.

> DocSourcePositions:57,89
> replace `CompilationUnit` with `compilation unit`

Replaced.

> DocTreeFactory:112
> We are inconsistent about whether to use `{..}` for inline tags, which will be worse when we add our first bimodal standard tag, `@return`, but using `{...}` for `{@deprecated}` is definitely wrong

Yes, I have that thought too. I fixed "@deprecated" along with other block tags in DocTreeFactory.java. On the other hand, I added missing curly braces around "@summary".

> General: (question)
> Does a trailing space inside `{@code something }` cause extra whitespace in the rendered HTML?

Yes, it does. I removed those *outer* trailing whitespace characters.

> DocTreePath:37,96,98,99
> More un-code-d type names. Follow existing conventions for either using `{@code...}` or the descriptive equivalent (e.g. "tree path")

@code-d those and some others.

> Trees:239,240
> replace "The" by "the"

Replaced there and in one other place.

> remove package name from TypeMirror, and use @code for that and ErrorType

Removed the package, but will NOT do the rest: it would be completely inconsistent with everything else in that file.

> DocCommentParser:
> avoid Latin abbreviations; use "for example, such as"

Fixed. Although, I don't get the rationale behind this piece of advice. One reason not to use any abbreviations would be the period characters: periods interfere with detecting first sentences in doc comments.

> 1103: another candidate for `{@spec}` !!

Yes. I just felt I have to provide something more fresh and immediately accessible.

> Pretty: removing import
> In general, this is a dangerous edit; throughout javac, you will see imports of javac.util.List in preference to java.util.List. I assume you have compiled and run the tests, to validate this change.

Correct, before posting the initial webrev I ran the tiers 1 through 3 in our CI. It was fine.

> WriterFactory:135
> Trailing period.

Removed there and in some other places.

> BaseTaglet
> Yay for removing redundant doc comments from overridden methods!
> 
> toolkit/taglets/Taglet.java
> 41,143: 
> If you think "tag" refers to instances in doc comments, the original was correct
> If you think "tag" refers to the class of the instances, the new text should use "the block tag"

Those terms have to be better defined in the spec.

> 142: consider remove "all" from end of line

Removed.

-Pavel

> -- Jon
> 