Thanks for having a good look at it, Jon. My replies are inline.
> On 14 Aug 2020, at 17:49, Jonathan Gibbons <[email protected]>
> 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
>
