On Thu, 26 Oct 2023 23:29:00 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
I'll review this PR's tests first. Most of them look good. For the rest,
comments inline.
test/langtools/jdk/javadoc/tool/sampleapi/lib/sampleapi/generator/ModuleGenerator.java
line 49:
> 47: import sampleapi.util.PoorDocCommentTable;
> 48:
> 49: import static
> com.sun.tools.javac.parser.Tokens.Comment.CommentStyle.JAVADOC;
To clarify, the change to this file is that you removed two unused imports,
right?
test/langtools/jdk/javadoc/tool/testTransformer/TestTransformer.java line 37:
> 35:
> 36: import java.nio.file.Path;
> 37: import java.util.ArrayList;
Import in unused:
Suggestion:
test/langtools/jdk/javadoc/tool/testTransformer/TestTransformer.java line 61:
> 59: }
> 60:
> 61: ToolBox tb = new ToolBox();
Suggestion:
private final ToolBox tb = new ToolBox();
test/langtools/jdk/javadoc/tool/testTransformer/TestTransformer.java line 64:
> 62:
> 63: @Test
> 64: public void testFindStandardTransformer_raw() throws Exception {
Checked exceptions are not thrown:
Suggestion:
public void testFindStandardTransformer_raw() {
test/langtools/jdk/javadoc/tool/testTransformer/TestTransformer.java line 82:
> 80:
> 81: @Test
> 82: public void testFindStandardTransformer_stream() throws Exception {
Checked exceptions are not thrown here too:
Suggestion:
public void testFindStandardTransformer_stream() {
test/langtools/jdk/javadoc/tool/testTransformer/TestTransformer.java line 96:
> 94: var sl =
> ServiceLoader.load(DocTrees.DocCommentTreeTransformer.class);
> 95: return StreamSupport.stream(sl.spliterator(), false)
> 96: .filter(p -> p.name().equals(name))
ServiceLoader specification suggests another way of streaming:
Suggestion:
return sl.stream()
.map(ServiceLoader.Provider::get)
.filter(t -> t.name().equals(name))
Would it be the same and more readable?
test/langtools/tools/javac/classfiles/attributes/deprecated/DeprecatedTest.java
line 26:
> 24: /*
> 25: * @test
> 26: * @bug 8042261 8298405
This comment is not for this line, but for two compiler tests for `@deprecated`
javadoc tag. It seemed quite contextual place to put it.
Did I miss it, or you are planning to add a javadoc test that verifies that
`@deprecated` appearing in a `///` comment has no [special meaning]() it has in
classic `/** */` comments?
[special meaning]:
https://github.com/openjdk/jdk/blob/128363bf3b57dfa05b3807271b47851733c1afb9/src/jdk.compiler/share/classes/com/sun/tools/javac/parser/JavaTokenizer.java#L1639-L1653
test/langtools/tools/javac/doctree/DocCommentTester.java line 1012:
> 1010: // }
> 1011: // return null;
> 1012: // }
Debugging leftover?
test/langtools/tools/javac/doctree/FirstSentenceTest.java line 423:
> 421: DocComment[DOC_COMMENT, pos:0
> 422: firstSentence: 1
> 423: RawText[MARKDOWN, pos:0, abc.|def.]
It took me a while to understand why this test expects the first sentence to
include the second line:
///abc.
///def.
void simpleMarkdown() { }
It's not because it's Markdown or the new `///` comment syntax. It's because
the breakiterator thinks that `abc.\ndef.` is one sentence.
Now, in this same file, on [line
123](https://github.com/openjdk/jdk/blob/128363bf3b57dfa05b3807271b47851733c1afb9/test/langtools/tools/javac/doctree/FirstSentenceTest.java#L119-L142),
there's this test:
/**
* abc def ghi.
* jkl mno pqr
*/
void dot_newline() { }
If you compare its expectation to that of simpleMarkdown(), you'll see that the
first sentence consists of the first line only:
BREAK_ITERATOR
DocComment[DOC_COMMENT, pos:1
firstSentence: 1
Text[TEXT, pos:1, abc_def_ghi.]
body: 1
Text[TEXT, pos:15, jkl_mno_pqr]
block tags: empty
]
So, why does it seem to work differently for `///` and `/** */` comments? It
turns out that a whitespace character immediately after newline is important
for breakiterator to do its thing:
```
abc def ghi.\n jkl mno pqr
^
The problem with the simpleMarkdown test is that we cannot just indent the
comment. That indentation will be deemed incidental whitespace and, thus, will
removed. For example, suppose we do this:
/// abc.
/// def.
void simpleMarkdown() { }
The breakiterator will still see `abc.\ndef.`. Of course, we could do this:
///abc.
/// def.
void simpleMarkdown() { }
But it would be a different comment.
There's another test in this PR,
[TestMarkdown.testFirstSentence](https://github.com/openjdk/jdk/blob/128363bf3b57dfa05b3807271b47851733c1afb9/test/langtools/jdk/javadoc/doclet/testMarkdown/TestMarkdown.java#L68-L83).
There, the first sentence consists only of the first line. This is likely
because the second line starts with a capital letter.
test/langtools/tools/javac/doctree/MDPrinter.java line 67:
> 65: * Conceptually based on javac's {@code DPrinter}.
> 66: */
> 67: public class MDPrinter {
While DPrinter is used, MDPrinter isn't. (At least, I could't find any usages
of it.) If you feel like MDPrinter is important, we should at least add a
compile test for it, to protect it from bitrot.
test/langtools/tools/javac/doctree/MarkdownTest.java line 464:
> 462: ///{@summary abc ``code-span {@dummy ...}`` def {@code ...} }
> 463: ///rest.
> 464: void codeSpanInInlineTag() { }
Initially, I thought that there were two empty code spans or perhaps two pairs
of literal backticks. But it turns out that CommonMark actually
[allows](https://spec.commonmark.org/0.30/#code-spans) this syntax for code
spans:
``codespan``
I guess I've never had to insert a backtick inside a code span, or it was done
differently in that flavour of Markdown I used.
test/langtools/tools/javac/doctree/MarkdownTest.java line 555:
> 553: // block tags: empty
> 554: //]
> 555: //*/
Just to clarify: it is supposed to be commented out, right? If uncommented,
this test fails with a slightly different error.
test/langtools/tools/javac/lexer/CommentTest.java line 24:
> 22: */
> 23:
> 24: /**
Suggestion:
/*
Otherwise, some IDEs confuse this jtreg comment for a javadoc comment.
test/langtools/tools/javac/lexer/CommentTest.java line 37:
> 35: import java.util.Objects;
> 36:
> 37: import com.sun.tools.javac.parser.JavadocTokenizer;
Unused import:
Suggestion:
test/langtools/tools/javac/lexer/CommentTest.java line 73:
> 71:
> 72: /**
> 73: * Whitespace before the commennt is completely ignored.
Typo:
Suggestion:
* Whitespace before the comment is completely ignored.
-------------
Changes requested by prappo (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/16388#pullrequestreview-1719995947
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1386961537
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1386970114
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1386969740
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1386971152
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1386974907
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1386985044
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1386892431
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1386854434
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1386442552
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1386406680
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1386485173
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1386487676
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1386410910
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1386411725
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1386413193
