On Mon, 8 Jan 2024 21:26:50 GMT, Jonathan Gibbons <j...@openjdk.org> wrote:
>> Please review a patch to add support for Markdown syntax in documentation
>> comments, as described in the associated JEP.
>>
>> Notable features:
>>
>> * support for `///` documentation comments in `JavaTokenizer`
>> * new module `jdk.internal.md` -- a private copy of the `commonmark-java`
>> library
>> * updates to `DocCommentParser` to treat `///` comments as Markdown
>> * updates to the standard doclet to render Markdown comments in HTML
>
> 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 seven additional
> commits since the last revision:
>
> - Merge with upstream/master
> - Merge remote-tracking branch 'upstream/master' into
> 8298405.doclet-markdown-v3
> - Address review comments
> - Fix whitespace
> - Improve handling of embedded inline taglets
> - Customize support for Markdown headings
> - JDK-8298405: Support Markdown in Documentation Comments
Here's another batch of comments. Please update `@since 22` to `@since 23`
throughout this PR.
src/jdk.compiler/share/classes/com/sun/source/doctree/DocTreeVisitor.java line
257:
> 255: *
> 256: * @implSpec Visits the provided {@code RawTextTree} node
> 257: * by calling {@code visitOther(node, p)}.
Nit: for consistency with the rest of the file, reorder `@param`-`@return`
block with the `@implSpec` tag:
Suggestion:
*
* @implSpec Visits the provided {@code RawTextTree} node
* by calling {@code visitOther(node, p)}.
*
* @param node the node being visited
* @param p a parameter value
* @return a result value
src/jdk.compiler/share/classes/com/sun/source/doctree/RawTextTree.java line 40:
> 38: * @apiNote
> 39: * This class may be used to represent tree nodes containing
> 40: * {@linkplain DocTree.Kind#MARKDOWN Markdown} text.
This means that there is one-to-many relationship between `RawTextTree` and
`DocTree.KIND`. This in turn perpetuates the pattern of checking the kind
followed by casting as opposed to more modern `instanceof` pattern matching.
There's nothing wrong with it per se, however I wonder what the rationale is
for leaving this part of the API open-ended. Is it to support other types of
raw text in the future?
src/jdk.compiler/share/classes/com/sun/source/util/DocTreeFactory.java line 299:
> 297: * @param code the code
> 298: * @return a {@code RawTextTree} object
> 299: * @throws IllegalArgumentException if the kind is not a recognized
> kind for raw text
This method's implementation also throws `NullPointerException` if kind is
null, which is unusual for these methods. You can either add `@throws`, or
workaround it by using `String.valueOf(kind)` instead of `kind.toString()` down
in the implementation.
src/jdk.compiler/share/classes/com/sun/source/util/DocTreeFactory.java line 303:
> 301: * @since 22
> 302: */
> 303: RawTextTree newRawTextTree(DocTree.Kind kind, String code) throws
> IllegalArgumentException;
It's unusual for a JDK method to declare a runtime exception.
src/jdk.compiler/share/classes/com/sun/source/util/DocTrees.java line 97:
> 95: */
> 96: public enum CommentKind {
> 97: /** The style of comments whose lines are prefixed by{@code ///}.
> */
Suggestion:
/** The style of comments whose lines are prefixed by {@code ///}. */
src/jdk.compiler/share/classes/com/sun/source/util/DocTrees.java line 104:
> 102:
> 103: /**
> 104: * {@return the style of the documentation comment associated with a
> tree node.}
Period is redundant:
Suggestion:
* {@return the style of the documentation comment associated with a tree
node}
src/jdk.compiler/share/classes/com/sun/source/util/DocTrees.java line 111:
> 109: * @since 22
> 110: */
> 111: public abstract CommentKind getDocCommentKind(TreePath path);
This method's specification says nothing about `null` that the implementation
can return.
src/jdk.compiler/share/classes/com/sun/source/util/DocTrees.java line 157:
> 155: * @param fileObject the content container
> 156: * @return the doc comment tree
> 157: * @throws IllegalArgumentException if the file type is not supported
It seems like this exception could've been thrown before, it's just that you
have documented it for the first time. This might be important for CSR.
src/jdk.compiler/share/classes/com/sun/source/util/DocTrees.java line 182:
> 180: * @return the doc comment tree
> 181: * @throws IOException if an exception occurs
> 182: * @throws IllegalArgumentException if the file type is not supported
Ditto on this newly documented exception.
src/jdk.compiler/share/classes/com/sun/source/util/DocTrees.java line 193:
> 191: *
> 192: * Supported file types are HTML files and Markdown files.
> 193: * Future releases may support additional file types.
Good catch on this missing statement, which is present in other similar methods.
src/jdk.compiler/share/classes/com/sun/source/util/DocTrees.java line 297:
> 295:
> 296: /**
> 297: * A functional interface to transform a {@code DocCommentTree}.
Re: functional interface
If you mean it, annotate it with `@FunctionalInterface`. If you don't, simply
remove that phrase.
src/jdk.compiler/share/classes/com/sun/source/util/DocTrees.java line 309:
> 307: * The standard implementation of this interface supports an
> extended form
> 308: * of reference links in Markdown comments, such that if the label
> for a
> 309: * reference link is undefined and matches a reference to a program
Suggestion:
* reference link is undefined and resembles a reference to a program
src/jdk.compiler/share/classes/com/sun/source/util/DocTrees.java line 329:
> 327: * Transforms a documentation comment tree.
> 328: *
> 329: * @param trees an instance of the {@link Trees} utility
> interface.
Suggestion:
* @param trees an instance of the {@link Trees} utility interface
src/jdk.compiler/share/classes/com/sun/tools/javac/api/JavacTrees.java line 992:
> 990:
> 991: private static boolean isMarkdownFile(FileObject fo) {
> 992: return fo.getName().endsWith(".md");
I wonder why you decided to (re)implement those methods using file extension
matching. Is it because we don't want to introduce anything Markdown-related to
this method that was used to implement `isHtmlFile` previously?
https://github.com/openjdk/jdk/blob/8eb4e7e07e9211aabcb0f22696e9c572dac7a59f/src/jdk.compiler/share/classes/com/sun/tools/javac/file/BaseFileManager.java#L489-L498
src/jdk.compiler/share/classes/com/sun/tools/javac/api/JavacTrees.java line
1080:
> 1078: }
> 1079:
> 1080: private String info(FileObject fo) {
This does not seem to be used; is it for debugging? If so, add your `// DEBUG`
comment.
src/jdk.compiler/share/classes/com/sun/tools/javac/api/JavacTrees.java line
1156:
> 1154:
> 1155: /**
> 1156: * {@return the {@linkplain ParserFactory} parser factory}.
Suggestion:
* {@return the {@linkplain ParserFactory} parser factory}
src/jdk.compiler/share/classes/com/sun/tools/javac/parser/DocCommentParser.java
line 89:
> 87: POSTAMBLE,
> 88: /** The rich-text content of an inline documentation comment tag.
> */
> 89: INLINE
Do we also need to say something about `.md` files here?
src/jdk.compiler/share/classes/com/sun/tools/javac/parser/DocCommentParser.java
line 130:
> 128:
> 129: /**
> 130: * Create a parser for a documentation comment.
Suggestion:
* Creates a parser for a documentation comment.
src/jdk.compiler/share/classes/com/sun/tools/javac/parser/DocCommentParser.java
line 201:
> 199: }
> 200: newline = true;
> 201: }
I can see clean LF and CRLF, but no FF; was the latter intentional?
In any case, we should double-check the generated test output to see if there's
anything unexpected.
src/jdk.compiler/share/classes/com/sun/tools/javac/parser/DocCommentParser.java
line 215:
> 213: case '\n', '\r' -> {
> 214: return newString(bp, p);
> 215: }
Hm... this does not seem to be consistent with `newline` in `nextChar`; should
it be consistent?
src/jdk.compiler/share/classes/com/sun/tools/javac/parser/DocCommentParser.java
line 241:
> 239: * <li>{@code PREAMBLE}: the appearance of {@code <body>} (or {@code
> <main>}),
> 240: * as determined by {@link #isEndPreamble()}
> 241: * <li>{@code BODY}: the beginning of a block tag, or when readung
> from
Suggestion:
* <li>{@code BODY}: the beginning of a block tag, or when reading from
src/jdk.compiler/share/classes/com/sun/tools/javac/parser/DocCommentParser.java
line 249:
> 247: *
> 248: *
> 249: *
Suggestion:
src/jdk.compiler/share/classes/com/sun/tools/javac/parser/DocCommentParser.java
line 1000:
> 998: * <li>cdata: {@code <![CDATA[ ... ]]>}
> 999: * </ul>
> 1000: * or
Dangling "or".
src/jdk.compiler/share/classes/com/sun/tools/javac/parser/JavaTokenizer.java
line 1065:
> 1063:
> 1064: if (accept('/')) { // (Spec. 3.7)
> 1065: if (accept('/')) { // Markdown comment
I believe that some of the changes in `com/sun/tools/javac/parser` were
contributed by @JimLaskey. If so, don't forget to add him as a co-author:
`/contributor add jlaskey`.
-------------
PR Review: https://git.openjdk.org/jdk/pull/16388#pullrequestreview-1818383901
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1451611443
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1450636952
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1452278438
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1450595553
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1450524385
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1450644783
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1450706952
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1450697324
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1450708532
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1450712757
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1450721951
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1450716256
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1450724201
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1450753241
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1450756727
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1450759909
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1452387246
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1452310521
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1452316008
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1452319517
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1452329926
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1452327712
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1452322576
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1450780454
