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.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