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>
>    *    {&#064;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  *    &lt;!doctype text&gt;
>> 
>>  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

Reply via email to