On Tue, 8 Dec 2020 13:14:19 GMT, Pavel Rappo <[email protected]> wrote:
>> Jonathan Gibbons has updated the pull request with a new target base due to
>> a merge or a rebase. The incremental webrev excludes the unrelated changes
>> brought in by the merge/rebase. The pull request contains 12 additional
>> commits since the last revision:
>>
>> - Merge remote-tracking branch 'upstream/master' into new-return
>> - Update JShell to handle inline `{@return}`
>> - Merge remote-tracking branch 'upstream/master' into new-return
>> - fix test
>> - Update for new `@return` tag
>> - Merge remote-tracking branch 'upstream/master' into new-return
>> - Update DocCommentParser to permit nested inline tags in specified cases:
>> @return
>> - Add default impl for new method
>> - Fix test failure
>> - Fix trailing whitespace in test
>> - ... and 2 more:
>> https://git.openjdk.java.net/jdk/compare/c3337822...a25dadca
>
> src/jdk.compiler/share/classes/com/sun/source/util/DocTreeFactory.java line
> 277:
>
>> 275: * @param description the description of the return value of a
>> method
>> 276: * @return a {@code ReturnTree} object
>> 277: * @throws UnsupportedOperationException if inline {@code {@return}
>> } tags are
>
> To be consistent with the rest of the file, I suggest using `{@code {@return
> }}` instead of `{@code {@return} }` (note the position of the whitespace).
fixed
> src/jdk.compiler/share/classes/com/sun/tools/javac/parser/DocCommentParser.java
> line 275:
>
>> 273: return tp.parse(p, TagParser.Kind.BLOCK);
>> 274: } else {
>> 275: return erroneous("dc.bad.inline.tag", p);
>
> This indentation fits the current `switch` but not the proposed `if`
> statement.
fixed
> src/jdk.compiler/share/classes/com/sun/source/doctree/ReturnTree.java line 44:
>
>> 42: * Returns whether this instance is an inline tag.
>> 43: *
>> 44: * @return {@code true} if this instance is an inline tag
>
> Although I'm not sure how `boolean` queries are generally specified in the
> `DocTree` API, consider explicitly specifying that this method returns
> `false` if called on a non-inline instance.
hmm, OK, seems a bit redundant but will add `and {@code false} otherwise`
> src/jdk.compiler/share/classes/com/sun/tools/javac/parser/DocCommentParser.java
> line 333:
>
>> 331: DCTree text =
>> inlineText(WhitespaceRetentionPolicy.REMOVE_ALL); // skip content
>> 332: nextChar();
>> 333: return m.at(p).newUnknownInlineTagTree(name,
>> List.of(text)).setEndPos(bp);
>
> There's a discrepancy between the inline comment and code. While the comment
> says that the `else` clause handles a block tag, the returned tree
> corresponds to an (unknown) inline tag.
The code is correct, and even the comment: which refers to "block tags in
inline content".
The case being handled is one like this:
`
/**
* This is a bad comment. {@see Object} or {@since 7}.
*/
`
> src/jdk.compiler/share/classes/com/sun/tools/javac/parser/DocCommentParser.java
> line 1132:
>
>> 1130:
>> 1131: /**
>> 1132: * @see <a
>> href="https://docs.oracle.com/en/java/javase/14/docs/specs/javadoc/doc-comment-spec.html";>JavaDoc
>> Tags</a>
>
> Consider updating the version to 15. Later, we'd hopefully be able to use the
> [`@spec` tag](https://bugs.openjdk.java.net/browse/JDK-8226279) here.
done
Yes, this could eventually be a use of the proposed `@spec` tag with a relative
URL
> src/jdk.compiler/share/classes/com/sun/tools/javac/tree/DCTree.java line 690:
>
>> 688:
>> 689: @Override
>> 690: public String getTagName() {
>
> Isn't this method missing `@DefinedBy(Api.COMPILER_TREE)`?
Oops, yes; fixed; I guess I'm mildly surprised the checker didn't catch it, but
that's a different issue
> src/jdk.compiler/share/classes/jdk/internal/shellsupport/doc/JavadocFormatter.java
> line 268:
>
>> 266: /**
>> 267: * {@inheritDoc}
>> 268: * {@code @return} is a bimodfal tage and can be used as either
>> a block tag or an inline
>
> Fix the typos in "bimodfal tage".
Oops, also fixed an instance of `null` to `{@code null}`
> test/langtools/jdk/internal/shellsupport/doc/JavadocFormatterTest.java line
> 140:
>
>> 138: "1234 1234 1234 1234 1234 1234 1234 1234 1234 1234
>> 1234 1234 1234\n" +
>> 139: "1234 1234 1234 1234 1234 1234 1234 1234 1234 1234
>> 1234 1234 1234\n" +
>> 140: "1234 1234 1234 1234 1234 1234 1234 1234 1234 \n";
>
> What's that about? I mean is it related to this work on `{@return}`?
It was needed to fix a test failure
-------------
PR: https://git.openjdk.java.net/jdk/pull/1355
