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
