RFR: JDK-8298405: Support Markdown in Documentation Comments [v7]

[Pavel Rappo]

On Fri, 19 Jan 2024 18:37:48 GMT, Jonathan Gibbons <jjg at 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.internal.md/share/classes/jdk/internal/markdown/MarkdownTransformer.java line 221:

> 219:                         }
> 220:                         String code = t.getContent();
> 221:                         // handle the (unlikely) case of FFFC characters existing in the code

For consistency with the rest of the file:
Suggestion:

                        // handle the (unlikely) case of U+FFFC characters existing in the code

src/jdk.internal.md/share/classes/jdk/internal/markdown/MarkdownTransformer.java line 230:

> 228:                             start = pos + 1;
> 229:                         }
> 230:                         sourceBuilder.append(code.substring(start));

If I understood this correctly, it could be achieved simpler:
Suggestion:

                            replacements.add(PLACEHOLDER);
                            start = pos + 1;
                        }
                        sourceBuilder.append(code);

src/jdk.internal.md/share/classes/jdk/internal/markdown/MarkdownTransformer.java line 353:

> 351:             return (equal(desc2, tree.description))
> 352:                     ? tree
> 353:                     : m.at(tree.pos).newReturnTree(tree.inline, desc2).setEndPos(tree.getEndPos());

Don't we need to set end position here only if the tag is in its inline variant?

src/jdk.internal.md/share/classes/jdk/internal/markdown/MarkdownTransformer.java line 487:

> 485:     }
> 486: 
> 487:     private static final String AUTOREF_PREFIX = "code:";

I wish the prefix were such that it could not be forged: for example, automatically assigned, unique within a document comment.

src/jdk.internal.md/share/classes/jdk/internal/markdown/MarkdownTransformer.java line 543:

> 541:                 @Override
> 542:                 public LinkReferenceDefinition getLinkReferenceDefinition(String label) {
> 543:                     var l = label.replace("\\[\\]", "[]");

That `String.replace` looks suspicious. FWIW, when I removed that `replace`, no tests failed.

src/jdk.internal.md/share/classes/jdk/internal/markdown/MarkdownTransformer.java line 559:

> 557:         private boolean isReference(String s) {
> 558:             try {
> 559:                 var ref = refParser.parse(s, ReferenceParser.Mode.MEMBER_OPTIONAL);

Suggestion:

                refParser.parse(s, ReferenceParser.Mode.MEMBER_OPTIONAL);

src/jdk.internal.md/share/classes/jdk/internal/markdown/MarkdownTransformer.java line 771:

> 769:                 copyTo(getStartPos(link));
> 770:                 // push temporary value for {@code trees} while handling the content of the node
> 771:                 var saveTrees = trees;

"saveTrees": I see what you did there :-)

src/jdk.internal.md/share/classes/jdk/internal/markdown/MarkdownTransformer.java line 882:

> 880:         private int getStartPos(Node node) {
> 881:             var spans = node.getSourceSpans();
> 882:             var firstSpan = spans.get(0);

Suggestion:

            var firstSpan = spans.getFirst();

src/jdk.internal.md/share/classes/jdk/internal/markdown/MarkdownTransformer.java line 894:

> 892:         private int getEndPos(Node node) {
> 893:             var spans = node.getSourceSpans();
> 894:             var lastSpan = spans.get(spans.size() - 1);

Suggestion:

            var lastSpan = spans.getLast();

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1465455477
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1465591498
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1465400705
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1465628293
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1465625839
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1465626080
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1466197262
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1465642275
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1465642491