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