I forgot to add the link to the new webrev. Here it is: http://cr.openjdk.java.net/~prappo/8251550/webrev.01/
> On 17 Aug 2020, at 13:30, Pavel Rappo <pavel.ra...@oracle.com> wrote: > > 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