Thanks Jon. I will wait for you to revert back to me.
Separately, here's one more typo:
@@ -90,7 +90,7 @@ public interface DocSourcePositions extends SourcePositions {
* @param comment the comment tree that encloses the tree for which the
* position is being sought
* @param tree tree for which a position is sought
- * @return the start position of tree
+ * @return the end position of tree
*/
long getEndPosition(CompilationUnitTree file, DocCommentTree comment,
DocTree tree);
I'll add that to the patch.
-Pavel
> On 17 Aug 2020, at 15:28, Jonathan Gibbons <[email protected]>
> wrote:
>
> I've read your responses, which all look OK.
>
> I'll give the webrev a look as well.
>
> -- Jon
>
> On 8/17/20 5:30 AM, Pavel Rappo wrote:
>> 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
>>>
