On Mon, 7 Dec 2020 18:37:37 GMT, Jonathan Gibbons <[email protected]> wrote:
>> This change extends the functionality of the `@return` tag so that it can
>> also be used as an inline tag in the first sentence of a description.
>>
>> The goal is to be able to simplify the following common pattern:
>>
>> /**
>> * Returns the result. Optional additional text.
>> * @return the result
>> */
>> int method() {
>>
>> by
>>
>> /**
>> * {@return the result} Optional additional text.
>> */
>> int method() {
>>
>> Note:
>>
>> * The inline tag may only be used at the beginning of the description. A
>> warning will be given if it is used elsewhere.
>> * The expansion of the inline tag is `Returns " _content_ `.` where
>> _content_ is the content of the tag.
>> * If there is no block `@return` tag, the standard doclet will look for an
>> inline tag at the beginning of the description
>> * The inline tag can be inherited into overriding methods as if it was
>> provided as a block tag.
>
> 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/8343a9ff...a25dadca
1. Although the change generally looks good, there are some issues that should
be fixed.
2. I have NOT reviewed the `jdk.internal.shellsupport.doc` portion of the
change in detail; please ask someone else to do that.
3. When I run this change through our CI, the
`tools/javac/diags/CheckExamples.java` test fails consistently on all
platforms. Try to merge with the latest `master` to rule out this PR as a cause.
4. Consider updating the copyright years when addressing this feedback.
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).
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.
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.
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.
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.
src/jdk.compiler/share/classes/com/sun/tools/javac/parser/DocCommentParser.java
line 1126:
> 1124: return parse(pos);
> 1125: }
> 1126: DCTree parse(int pos) throws ParseException {
Please add an empty line between those two to separate the methods.
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)`?
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".
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclint/resources/doclint.properties
line 60:
> 58: dc.param.name.not.found = @param name not found
> 59: dc.ref.not.found = reference not found
> 60: dc.return.not.first = '{@return'} not at beginning of description
I can see an inconsistency here. While the templates for `@return` and `@code`
put an apostrophe immediately before the closing `}`, the template for `@value`
(further below in this file) puts an apostrophe immediately after the closing
`}`.
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}`?
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/ReturnTaglet.java
line 76:
> 74: } else {
> 75: List<? extends DocTree> firstSentence =
> utils.getFirstSentenceTrees(input.element);
> 76: if (firstSentence.size() == 1 &&
> firstSentence.get(0).getKind() == DocTree.Kind.RETURN) {
There's one more place where `firstSentence.size() == 1` is checked. I wonder
if it could be a simpler `!firstSentence.isEmpty()` check?
Are there any non-trivial cases which might bite us later, should we change
that check?
-------------
Changes requested by prappo (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/1355
