On Fri, 19 Jan 2024 18:37:48 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 pull request now contains eight commits:
>
> - Merge with upstream/master
> - 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
src/jdk.compiler/share/classes/com/sun/tools/javac/parser/DocCommentParser.java
line 225:
> 223: while (ch == ' ' && bp < buflen) {
> 224: nextChar();
> 225: }
Why do we specifically care about `\s` symbols here rather than about broader
whitespace?
src/jdk.compiler/share/classes/com/sun/tools/javac/parser/DocCommentParser.java
line 1165:
> 1163: }
> 1164:
> 1165:
Please delete this blank line.
src/jdk.compiler/share/classes/com/sun/tools/javac/parser/DocCommentParser.java
line 1315:
> 1313: }
> 1314:
> 1315: void skipLine() {
Like `peekLike()`, this method also does not seem to be consistent with
`newline` in `nextChar()`.
src/jdk.compiler/share/classes/com/sun/tools/javac/parser/DocCommentParser.java
line 1382:
> 1380: * @see <a
> href="https://spec.commonmark.org/0.30/#setext-headings";>Setext Headings</a>
> 1381: */
> 1382: SETEXT_UNDERLINE(Pattern.compile("[=-]+[ \t]*")),
This should be more accurate:
Suggestion:
SETEXT_UNDERLINE(Pattern.compile("=+|-+[ \t]*")),
src/jdk.compiler/share/classes/com/sun/tools/javac/parser/DocCommentParser.java
line 1388:
> 1386: * @see <a
> href="https://spec.commonmark.org/0.30/#thematic-breaks";>Thematic Break</a>
> 1387: */
> 1388: THEMATIC_BREAK(Pattern.compile("((\\+[ \t]*+){3,})|((-[
> \t]*+){3,})|((_[ \t]*+){3,})")),
Suggestion:
/**
* Thematic break: a line of * - _ interspersed with optional spaces
and tabs
* @see <a
href="https://spec.commonmark.org/0.30/#thematic-breaks";>Thematic Break</a>
*/
THEMATIC_BREAK(Pattern.compile("((\*[ \t]*+){3,})|((-[ \t]*+){3,})|((_[
\t]*+){3,})")),
src/jdk.compiler/share/classes/com/sun/tools/javac/parser/DocCommentParser.java
line 1396:
> 1394: * @see <a
> href="https://spec.commonmark.org/0.30/#code-fence";>Code Fence</a>
> 1395: */
> 1396: CODE_FENCE(Pattern.compile("(`{3,}[^`]*+)|(~{3,}.*+)")),
Why are this and the previous patterns possessive (`+`), while others aren't?
src/jdk.compiler/share/classes/com/sun/tools/javac/parser/DocCommentParser.java
line 1419:
> 1417: LineKind peekLineKind() {
> 1418: switch (ch) {
> 1419: case '#', '=', '-', '+', '_', '`', '~' -> {
This change is to match that of `THEMATIC_BREAK`:
Suggestion:
case '#', '=', '-', '*', '_', '`', '~' -> {
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
Here and elsewhere in this file: do we need to mention Markdown?
src/jdk.compiler/share/classes/com/sun/tools/javac/parser/JavaTokenizer.java
line 1553:
> 1551:
> 1552: /**
> 1553: * Determine how much indent to remove from markdown comment.
Suggestion:
* Determine how much indent to remove from Markdown comment.
src/jdk.compiler/share/classes/com/sun/tools/javac/parser/JavaTokenizer.java
line 1585:
> 1583: */
> 1584: UnicodeReader trimMarkdownComment(UnicodeReader line, int
> indent) {
> 1585: int pos = line.position();
Unused.
src/jdk.compiler/share/classes/com/sun/tools/javac/parser/ParserFactory.java
line 30:
> 28: import java.util.Locale;
> 29:
> 30: import com.sun.source.util.DocTrees;
Unused.
src/jdk.compiler/share/classes/com/sun/tools/javac/tree/DocPretty.java line 35:
> 33: import com.sun.source.doctree.*;
> 34: import com.sun.source.doctree.AttributeTree.ValueKind;
> 35: import com.sun.source.util.DocTreeScanner;
Unused.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1463169245
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1463155900
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1463252506
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1463181845
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1463202143
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1463237327
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1463239667
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1463422663
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1463422992
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1463424623
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1463437420
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1463441675
