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
