RFR: JDK-8075778: Add javadoc tag to avoid duplication of return information in simple situations. [v6]
Pavel Rappo
prappo at openjdk.java.net
Tue Dec 8 17:59:24 UTC 2020
On Mon, 7 Dec 2020 18:37:37 GMT, Jonathan Gibbons <jjg at openjdk.org> 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
More information about the compiler-dev
mailing list