Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v7]
On Thu, 8 Feb 2024 18:52:54 GMT, Jonathan Gibbons wrote:
>> src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/markup/RawHtml.java
>> line 145:
>>
>>> 143: }
>>> 144:
>>> 145: Pattern tag = Pattern.compile("<(?[A-Za-z0-9]+)(\\s|>)");
>>
>> I'm not sure I grok this pattern; what's up with `\\s`?
>
> The code is looking for HTML tag names, The match for a tag name is one of
> * `<` _tag-name_ `>`
> * `<` _tag-name_ _whitespace_
I see, thanks.
Suggestion:
private final Pattern tag = Pattern.compile("<(?[A-Za-z0-9]+)(\\s|>)");
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1486384478
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v7]
On Tue, 30 Jan 2024 23:47:00 GMT, Jonathan Gibbons wrote:
>> 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 :-)
>
> ?? Not sure I understand this comment.
It's just a funny word play evoking the famous eco slogan, is all.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1486368352
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v7]
On Thu, 1 Feb 2024 00:19:42 GMT, Jonathan Gibbons wrote: >> I'll add a test case. > > Done Thank you. I double-checked: if that `replace` is removed, jdk/javadoc/doclet/testMarkdown/TestMarkdown.java fails. - PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1486361794
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v7]
On Wed, 31 Jan 2024 22:15:23 GMT, Jonathan Gibbons wrote: >> I guess I don't see this as being as big a deal as you seem to think it is. >> What is it that you are so concerned about? >> >> I also think it is a potential feature to document and use reference links >> with `code:` URLs, using the reference link syntax to avoid having long >> method signatures in narrative text. >> >> For example, >> >>One of the methods on [List] has [lots of args][List.of10]. >> >>[List.of10]code:List.of(E,E,E,E,E,E,E,E,E,E) > > I've changed the prefix to be dynamically generated. It's now called > `autorefScheme` and is passed around as needed. It contains a hex hashcode, > so is reasonably unguessable. > > I'm still sort-of sad to lose the ability to use `code:` URLs directly, so > I've left a comment in the code hinting that that is a possibility. Thanks for accepting this; we could revert it later if the fixed, known scheme is deemed more useful. - PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1486364459
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v7]
On Tue, 30 Jan 2024 23:15:32 GMT, Jonathan Gibbons wrote:
>> src/jdk.compiler/share/classes/com/sun/tools/javac/tree/DocTreeMaker.java
>> line 790:
>>
>>> 788:
>>> 789: // end of paragraph is newline, followed by a blank line or
>>> the beginning of the next block
>>> 790: private static final Pattern endPara = Pattern.compile("\n(([
>>> \t]*\n)|( {0,3}[-+*#=]))");
>>
>> So DocTreeMaker now also knows about Markdown. I wonder if we can avoid
>> that. Also, I assume you mean this (`+` is not a part of "thematic break"):
>> Suggestion:
>>
>> private static final Pattern endPara = Pattern.compile("\n(([
>> \t]*\n)|( {0,3}[-_*#=]))");
>
> The code is doing its best to model the non-Markdown behavior, which is to
> detect paragraph breaks, which terminate the first sentence in the absence of
> any period.
>
> `+` is in the pattern as a list marker; I added `_` for thematic break, and
> added more comments.
I can see that you have fixed it to recognise lists `+` and thematic breaks
`_`; good.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1486343596
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v7]
On Tue, 23 Jan 2024 13:02:46 GMT, Pavel Rappo wrote:
>> 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 1315:
>
>> 1313: }
>> 1314:
>> 1315: void skipLine() {
>
> Like `peekLike()`, this method also does not seem to be consistent with
> `newline` in `nextChar()`.
It's okay. You explained my confusion here:
https://github.com/openjdk/jdk/pull/16388#discussion_r1470367971
> 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 '#', '=', '-', '*', '_', '`', '~' -> {
While you seem to have forgotten to change it, no tests failed, which is sad.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1484619952
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1484617917
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v7]
On Tue, 30 Jan 2024 21:37:10 GMT, Jonathan Gibbons wrote:
>> 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?
>
> The "M" word appears 10 times in this file. I'll work to convert them to an
> alternate nomenclature, such as "line comment".
I can see that you've managed to remove all of those occurrences nicely; well
done!
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1484624573
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v7]
On Fri, 26 Jan 2024 18:14:05 GMT, Pavel Rappo wrote:
>> 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.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/CommentUtils.java
> line 629:
>
>> 627: public DocCommentTree parse(URI uri, String text) {
>> 628: return trees.getDocCommentTree(new SimpleJavaFileObject(
>> 629: uri, JavaFileObject.Kind.HTML) {
>
> Was it a bug before?
I would describe it as having been a "latent bug, waiting to happen".
Previously, all file objects were regarded as containing HTML content, and so
there was no need to use the parameter, although maybe it would have been good
to check it.
Now, file objects can be HTML or Markdown, and so we do need to use the
parameter.
In the case here, the method is used on strings specified in command-line
options, which are specified to be in HTML format. Yes, we might want to
change that, but that would be a different RFE separate from the work in this
PR. In that future evolution, I would suggest adding a `JavaFileObject.Kind`
parameter to parse and/or inferring the kind from the tail of the path in the
`uri` parameter.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1483458107
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v7]
On Fri, 26 Jan 2024 18:10:18 GMT, Pavel Rappo wrote:
>> 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.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/markup/RawHtml.java
> line 145:
>
>> 143: }
>> 144:
>> 145: Pattern tag = Pattern.compile("<(?[A-Za-z0-9]+)(\\s|>)");
>
> I'm not sure I grok this pattern; what's up with `\\s`?
The match for a tag is one of
* `<` _tag-name_ `>`
* `<` _tag-name_ _whitespace_
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1483444658
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v7]
On Fri, 19 Jan 2024 18:37:48 GMT, Jonathan Gibbons 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.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/HtmlConfiguration.java
line 329:
> 327: if (doclint == null) {
> 328: var trees = docEnv.getDocTrees();
> 329: if (trees.getDocCommentTreeTransformer()== null) {
Suggestion:
if (trees.getDocCommentTreeTransformer() == null) {
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/markup/RawHtml.java
line 145:
> 143: }
> 144:
> 145: Pattern tag = Pattern.compile("<(?[A-Za-z0-9]+)(\\s|>)");
I'm not sure I grok this pattern; what's up with `\\s`?
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/CommentUtils.java
line 629:
> 627: public DocCommentTree parse(URI uri, String text) {
> 628: return trees.getDocCommentTree(new SimpleJavaFileObject(
> 629: uri, JavaFileObject.Kind.HTML) {
Was it a bug before?
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclint/Env.java line 139:
> 137: this.types = types;
> 138:
> 139: if (this.trees.getDocCommentTreeTransformer()== null) {
Suggestion:
if (this.trees.getDocCommentTreeTransformer() == null) {
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1467988621
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1467977203
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1467980609
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1467982493
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v7]
On Fri, 26 Jan 2024 16:33:08 GMT, Pavel Rappo wrote:
>> 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 668:
>
>> 666: * U+FFFC characters that were found in the original document.
>> 667: */
>> 668: Iterator replaceIter;
>
> Suggestion:
>
> final Iterator replaceIter;
actually, `private final` ... done
> src/jdk.internal.md/share/classes/jdk/internal/markdown/MarkdownTransformer.java
> line 732:
>
>> 730: offsets.add(m.end());
>> 731: }
>> 732: sourceLineOffsets =
>> offsets.stream().mapToInt(Integer::intValue).toArray();
>
> Here's an alternative, which you might find better (or not):
> Suggestion:
>
> sourceLineOffsets = Stream.concat(Stream.of(0),
> lineBreak.matcher(source).results()
>
> .map(MatchResult::end)).mapToInt(Integer::intValue).toArray();
done
it's borderline (to me) that it's worthwhile but given that my original code
used streams to create the final array, it sort-of makes sense to go "all in".
> src/jdk.internal.md/share/classes/jdk/internal/markdown/MarkdownTransformer.java
> line 831:
>
>> 829: /**
>> 830: * {@return the position in the original comment for a position
>> in {@code source},
>> 831: * using {@link #replaceAdjustPos}}.
>
> Suggestion:
>
> * using {@link #replaceAdjustPos}}
done
> src/jdk.internal.md/share/classes/jdk/internal/markdown/MarkdownTransformer.java
> line 939:
>
>> 937:
>> 938: /**
>> 939: * Flush any text in the {@code text} buffer, by creating a new
>
> Suggestion:
>
> * Flushes any text in the {@code text} buffer, by creating a new
done
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1473668136
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1473670108
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1473668952
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1473668548
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v7]
On Wed, 31 Jan 2024 23:09:57 GMT, Jonathan Gibbons wrote:
>> Yes, you need to escape `[` and `]` within the label of any Markdown
>> reference link, by preceding each with backslash. (Remember, the label is
>> the string used to find the URL for the link; not the displayed text of the
>> link).
>> That's a CommonMark feature independent of this work.
>>
>> While we could change that `replace` call into two separate ones, in
>> reference signatures they always appear together as a pair, and can be
>> replaced together. We need to remove the escape characters in the
>> generated URL so that the signature is a standard signature with unescaped
>> `[]`
>>
>> For fun/demo, try the following:
>>
>>
>> /// First sentence.
>> /// * Link 0 to [java.lang.Object]
>> /// * Link 1a to [Arrays-equals][java.util.Arrays#equals(Object[],Object[])]
>> /// * Link 1b to [Arrays-equals][java.util.Arrays#equals(Object[],Object[])]
>> /// * Link 2a to [java.util.Arrays#equals(Object[],Object[])]
>> /// * Link 2b to [java.util.Arrays#equals(Object[],Object[])]
>> public class C { }
>>
>>
>> Link 1a and 2a end up as unprocessed "literal" text (because the `[]` were
>> not escaped.) That is, they are not even recognized by the CommonMark
>> parser as reference links. Link 1b and 2b get processed as links, as
>> expected.
>>
>> FWIW, this issue with needing to escape `[]` pairs is specifically mentioned
>> in the JEP as an inescapable (sic) limitation.
>
> I'll add a test case.
Done
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1473644884
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v7]
On Wed, 31 Jan 2024 23:09:22 GMT, Jonathan Gibbons wrote:
>> My recollection is that you have to escape square brackets in a reference
>> label. I will investigate, and if necessary, add a test case.
>
> Yes, you need to escape `[` and `]` within the label of any Markdown
> reference link, by preceding each with backslash. (Remember, the label is
> the string used to find the URL for the link; not the displayed text of the
> link).
> That's a CommonMark feature independent of this work.
>
> While we could change that `replace` call into two separate ones, in
> reference signatures they always appear together as a pair, and can be
> replaced together. We need to remove the escape characters in the generated
> URL so that the signature is a standard signature with unescaped `[]`
>
> For fun/demo, try the following:
>
>
> /// First sentence.
> /// * Link 0 to [java.lang.Object]
> /// * Link 1a to [Arrays-equals][java.util.Arrays#equals(Object[],Object[])]
> /// * Link 1b to [Arrays-equals][java.util.Arrays#equals(Object[],Object[])]
> /// * Link 2a to [java.util.Arrays#equals(Object[],Object[])]
> /// * Link 2b to [java.util.Arrays#equals(Object[],Object[])]
> public class C { }
>
>
> Link 1a and 2a end up as unprocessed "literal" text (because the `[]` were
> not escaped.) That is, they are not even recognized by the CommonMark parser
> as reference links. Link 1b and 2b get processed as links, as expected.
>
> FWIW, this issue with needing to escape `[]` pairs is specifically mentioned
> in the JEP as an inescapable (sic) limitation.
I'll add a test case.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1473594252
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v7]
On Wed, 31 Jan 2024 22:10:03 GMT, Jonathan Gibbons wrote:
>> 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.
>
> My recollection is that you have to escape square brackets in a reference
> label. I will investigate, and if necessary, add a test case.
Yes, you need to escape `[` and `]` within the label of any Markdown reference
link, by preceding each with backslash. (Remember, the label is the string
used to find the URL for the link; not the displayed text of the link).
That's a CommonMark feature independent of this work.
While we could change that `replace` call into two separate ones, in reference
signatures they always appear together as a pair, and can be replaced together.
We need to remove the escape characters in the generated URL so that the
signature is a standard signature with unescaped `[]`
For fun/demo, try the following:
/// First sentence.
/// * Link 0 to [java.lang.Object]
/// * Link 1a to [Arrays-equals][java.util.Arrays#equals(Object[],Object[])]
/// * Link 1b to [Arrays-equals][java.util.Arrays#equals(Object[],Object[])]
/// * Link 2a to [java.util.Arrays#equals(Object[],Object[])]
/// * Link 2b to [java.util.Arrays#equals(Object[],Object[])]
public class C { }
Link 1a and 2a end up as unprocessed "literal" text (because the `[]` were not
escaped.) That is, they are not even recognized by the CommonMark parser as
reference links. Link 1b and 2b get processed as links, as expected.
FWIW, this issue with needing to escape `[]` pairs is specifically mentioned in
the JEP as an inescapable (sic) limitation.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1473593860
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v7]
On Tue, 30 Jan 2024 23:42:40 GMT, Jonathan Gibbons wrote: >> 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. > > I guess I don't see this as being as big a deal as you seem to think it is. > What is it that you are so concerned about? > > I also think it is a potential feature to document and use reference links > with `code:` URLs, using the reference link syntax to avoid having long > method signatures in narrative text. > > For example, > >One of the methods on [List] has [lots of args][List.of10]. > >[List.of10]code:List.of(E,E,E,E,E,E,E,E,E,E) I've changed the prefix to be dynamically generated. It's now called `autorefScheme` and is passed around as needed. It contains a hex hashcode, so is reasonably unguessable. I'm still sort-of sad to lose the ability to use `code:` URLs directly, so I've left a comment in the code hinting that that is a possibility. - PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1473532970
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v7]
On Wed, 24 Jan 2024 22:35:31 GMT, Pavel Rappo wrote:
>> 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 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.
My recollection is that you have to escape square brackets in a reference
label. I will investigate, and if necessary, add a test case.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1473528775
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v7]
On Tue, 30 Jan 2024 22:55:27 GMT, Jonathan Gibbons wrote: >> src/jdk.compiler/share/classes/com/sun/tools/javac/tree/DocTreeMaker.java >> line 238: >> >>> 236: && postamble.isEmpty() >>> 237: && fullBody.stream().anyMatch(t -> t.getKind() >>> == Kind.MARKDOWN) >>> 238: ? CommentStyle.JAVADOC_LINE : >>> CommentStyle.JAVADOC_BLOCK; >> >> While clever, it seems to be prone to false positive `JAVADOC_LINE`. Also, >> it is inconsistent with `null` and `Position.NOPOS` returned from the >> `getText` and `getSourcePos(int)` methods respectively. > > I'm not worried about false positive `JAVADOC_LINE` than maybe false > _negative_ `JAVADOC_LINE`. Generally, the underlying issue here is how to > handle weird combinations of `DocTrees` constructed by a user of the public > API. For example, should we check, and reject, a `fullBody` containing > `MARKDOWN` nodes and non-empty `preamble` or `postamble`, since that > combination will never come from `DocCommentParser`. > > I'm not worried about comparison with `getText` and `getSourcePos`, since > there really is no other value for the source text or source position that we > can return. But we can infer a best guess for the style. > > Arguably, we should check the `tags` as well. > > -- > > I dug deeper. > > We do create synthetic trees in the standard doclet, such as for the > descriptions of mandated methods (like `values` and `valueOf` for any enum > class. Those synthetic trees do utilize this code path, although the > `getStyle` method is not currently invoked. (Verified by changing code to > throw `UnsupportedOperationException` and run all the tests.) It's also true > those synthetic trees do not leverage any Markdown support at this time. > > I prefer to leave the code as-is at this time. I revisited this, and rewrote the code and added more comments for now and for later. - PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1473474540
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v7]
On Wed, 24 Jan 2024 22:38:58 GMT, Pavel Rappo wrote:
>> 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 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.
I guess I don't see this as being as big a deal as you seem to think it is.
What is it that you are so concerned about?
I also think it is a potential feature to document and use reference links with
`code:` URLs, using the reference link syntax to avoid having long method
signatures in narrative text.
For example,
One of the methods on [List] has [lots of args][List.of10].
[List.of10]code:List.of(E,E,E,E,E,E,E,E,E,E)
> src/jdk.internal.md/share/classes/jdk/internal/markdown/MarkdownTransformer.java
> line 763:
>
>> 761: *
>> 762: * @param link the link node
>> 763: */
>
> Suggestion:
>
> /**
> * Visits a {@code Link} commonmark node.
> *
> * If the destination for the link begins with {@code code:}
> * convert it to {@code {@link ...}} or {@code {@linkplain ...}} doc
> tree node.
> * {@code {@link ...}} will be used if the content (label) for
> * the link is the same as the reference found after the {@code
> code:};
> * otherwise, {@code {@linkplain ...}} will be used.
> *
> * The label will be left blank for {@code {@link ...}} nodes,
> * implying that a default label should be used, based on the
> * program element that was referenced.
> *
> * @param link the link node
> */
fixed
> 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 :-)
?? Not sure I understand this comment.
> src/jdk.internal.md/share/classes/jdk/internal/markdown/MarkdownTransformer.java
> line 778:
>
>> 776: copyTo(getEndPos(link.getLastChild()));
>> 777:
>> 778: // determine whether to use {@link... } or
>> {@linkplain ...}
>
> Nit:
> Suggestion:
>
> // determine whether to use {@link ... } or {@linkplain
> ...}
fixed
> src/jdk.internal.md/share/classes/jdk/internal/markdown/MarkdownTransformer.java
> line 840:
>
>> 838:
>> 839: /**
>> 840: * Process a node and any children.
>
> Suggestion:
>
> * Processes a node and any children.
fixed
> 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();
done
> 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();
Done
> src/jdk.internal.md/share/classes/jdk/internal/markdown/MarkdownTransformer.java
> line 950:
>
>> 948: }
>> 949:
>> 950:
>
> Suggestion:
fixed
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1472122002
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1472129024
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1472125185
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1472129294
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1472129971
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1472123360
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1472124337
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1472130370
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v7]
On Wed, 24 Jan 2024 19:43:51 GMT, Pavel Rappo wrote:
>> 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
Fixed
> 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);
clever!
> 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?
It has an `endPos` field; we might as well set it.
> 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);
ok
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1472107834
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1472111878
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1472106632
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1472112635
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v7]
On Wed, 24 Jan 2024 16:30:50 GMT, Pavel Rappo wrote:
>> 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/tree/DocTreeMaker.java
> line 790:
>
>> 788:
>> 789: // end of paragraph is newline, followed by a blank line or the
>> beginning of the next block
>> 790: private static final Pattern endPara = Pattern.compile("\n(([
>> \t]*\n)|( {0,3}[-+*#=]))");
>
> So DocTreeMaker now also knows about Markdown. I wonder if we can avoid that.
> Also, I assume you mean this (`+` is not a part of "thematic break"):
> Suggestion:
>
> private static final Pattern endPara = Pattern.compile("\n(([
> \t]*\n)|( {0,3}[-_*#=]))");
The code is doing its best to model the non-Markdown behavior, which is to
detect paragraph breaks, which terminate the first sentence in the absence of
any period.
`+` is in the pattern as a list marker; I added `_` for thematic break, and
added more comments.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1472103907
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v7]
On Wed, 24 Jan 2024 12:17:19 GMT, Pavel Rappo wrote: >> 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/tree/DocTreeMaker.java > line 238: > >> 236: && postamble.isEmpty() >> 237: && fullBody.stream().anyMatch(t -> t.getKind() >> == Kind.MARKDOWN) >> 238: ? CommentStyle.JAVADOC_LINE : >> CommentStyle.JAVADOC_BLOCK; > > While clever, it seems to be prone to false positive `JAVADOC_LINE`. Also, it > is inconsistent with `null` and `Position.NOPOS` returned from the `getText` > and `getSourcePos(int)` methods respectively. I'm not worried about false positive `JAVADOC_LINE` than maybe false _negative_ `JAVADOC_LINE`. Generally, the underlying issue here is how to handle weird combinations of `DocTrees` constructed by a user of the public API. For example, should we check, and reject, a `fullBody` containing `MARKDOWN` nodes and non-empty `preamble` or `postamble`, since that combination will never come from `DocCommentParser`. I'm not worried about comparison with `getText` and `getSourcePos`, since there really is no other value for the source text or source position that we can return. But we can infer a best guess for the style. Arguably, we should check the `tags` as well. -- I dug deeper. We do create synthetic trees in the standard doclet, such as for the descriptions of mandated methods (like `values` and `valueOf` for any enum class. Those synthetic trees do utilize this code path, although the `getStyle` method is not currently invoked. (Verified by changing code to throw `UnsupportedOperationException` and run all the tests.) It's also true those synthetic trees do not leverage any Markdown support at this time. I prefer to leave the code as-is at this time. > src/jdk.compiler/share/classes/com/sun/tools/javac/tree/DocTreeMaker.java > line 704: > >> 702: } >> 703: >> 704: // If the break is well within the span of the string i.e. >> not > > Oh irony! Sentence segmentation in javadoc has some problems with > abbreviations like that. This was fixed 10/26/23. Both forms of the sentence breaker are supposed to get this correct, but yes, chuckle, irony. - PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1472082408 PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1472089239
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v7]
On Tue, 23 Jan 2024 15:17:14 GMT, Pavel Rappo wrote: >> 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/ParserFactory.java > line 30: > >> 28: import java.util.Locale; >> 29: >> 30: import com.sun.source.util.DocTrees; > > Unused. removed > 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. removed - PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1472042122 PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1472042782
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v7]
On Mon, 22 Jan 2024 16:57:16 GMT, Pavel Rappo wrote:
>> 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 1377:
>
>> 1375: * @see > href="https://spec.commonmark.org/0.30/#atx-headings;>ATX Headings
>> 1376: */
>> 1377: ATX_HEADER(Pattern.compile("#{1,6}( .*|$)")),
>
> Actually, I wonder how accurate those regexes should match spec. Given the
> definition [^*] of an ATX header and the fact that we always match the
> complete (not find inside) a line, which by definition should not have line
> terminators, shouldn't it be like this?
>
> #{1,6}([ \t]+.*)?
>
> [^*]: The opening sequence of # characters must be followed by spaces or
> tabs, or by the end of line.
The spec says:
> The opening sequence of # characters must be followed by spaces or tabs, or
> by the end of line.
so we're both slightly wrong. :-)
I will fix to `"#{1,6}([ \t].*|$)"))`
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1472036765
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v7]
On Tue, 23 Jan 2024 15:07:15 GMT, Pavel Rappo wrote:
>> 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/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.
fixed
> 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.
Yes, and the method has no side effects. Removed.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1472018320
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1472020269
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v7]
On Tue, 23 Jan 2024 12:50:45 GMT, Pavel Rappo wrote:
>> 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 1396:
>
>> 1394: * @see > href="https://spec.commonmark.org/0.30/#code-fence;>Code Fence
>> 1395: */
>> 1396: CODE_FENCE(Pattern.compile("(`{3,}[^`]*+)|(~{3,}.*+)")),
>
> Why are this and the previous patterns possessive (`+`), while others aren't?
Trying to avoid the runaway performance problem, such as described here:
https://www.regular-expressions.info/catastrophic.html
> 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?
The "M" word appears 10 times in this file. I'll work to convert them to an
alternate nomenclature, such as "line comment".
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1471991081
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1471995407
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v7]
On Thu, 25 Jan 2024 12:41:02 GMT, Pavel Rappo wrote:
>> src/jdk.compiler/share/classes/com/sun/tools/javac/parser/DocCommentParser.java
>> line 1388:
>>
>>> 1386: * @see >> href="https://spec.commonmark.org/0.30/#thematic-breaks;>Thematic Break
>>> 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 > href="https://spec.commonmark.org/0.30/#thematic-breaks;>Thematic Break
>> */
>> THEMATIC_BREAK(Pattern.compile("((\*[ \t]*+){3,})|((-[
>> \t]*+){3,})|((_[ \t]*+){3,})")),
>
> To add to my earlier [comment], DocCommentParser recognises THEMATIC_BREAK
> consisting of `-` as SETEXT_UNDERLINE. While it's inaccurate, it doesn't seem
> important, as DCP's goal is to recognise and avoid Markdown, not process it.
>
> [comment]: https://github.com/openjdk/jdk/pull/16388/files#r1462148038
Added lots more comment to the `LineKind` enum.
For the record, I would rephrase your statement about DCP's goal as follows:
_DCP's goal is to recognize block boundaries when skipping over literal text,
such as a code span; the specific kind of boundary generally does not matter._
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1471973350
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v7]
On Tue, 23 Jan 2024 12:01:38 GMT, Pavel Rappo wrote:
>> 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?
Good catch and a tricky question. It's a conflict between typically skipping
whitespace after a tag name (in traditional comments) and leading whitespace
being significant in Markdown text.
Resolved in a compromise by using `isHorizontalWhiteSpace(c)` and adding an
explanatory comment.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1471950685
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v7]
On Mon, 22 Jan 2024 15:35:41 GMT, Pavel Rappo wrote:
>> 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 271:
>
>> 269: //if (textStart == -1) {
>> 270: //textStart = bp;
>> 271: //}
>
> What's up with that?
It was a temporary senior moment, attempting to trim the content of the comment
-- but the leading whitespace is significant and should not be removed. The
lines have been removed.
> src/jdk.compiler/share/classes/com/sun/tools/javac/parser/DocCommentParser.java
> line 1165:
>
>> 1163: }
>> 1164:
>> 1165:
>
> Please delete this blank line.
Done
> src/jdk.compiler/share/classes/com/sun/tools/javac/parser/DocCommentParser.java
> line 1333:
>
>> 1331: lineKind = (ch == '\n' || ch == '\r') ?
>> LineKind.BLANK
>> 1332: : (indent <= 3) ? peekLineKind()
>> 1333: : LineKind.OTHER;
>
> Nested ternary-s are hard to read. Nested if-s are bulky. Sigh.
That was the neatest I could make it ;-)
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1471829709
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1471831762
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1471830921
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v7]
On Fri, 19 Jan 2024 18:37:48 GMT, Jonathan Gibbons 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 668:
> 666: * U+FFFC characters that were found in the original document.
> 667: */
> 668: Iterator replaceIter;
Suggestion:
final Iterator replaceIter;
src/jdk.internal.md/share/classes/jdk/internal/markdown/MarkdownTransformer.java
line 732:
> 730: offsets.add(m.end());
> 731: }
> 732: sourceLineOffsets =
> offsets.stream().mapToInt(Integer::intValue).toArray();
Here's an alternative, which you might find better (or not):
Suggestion:
sourceLineOffsets = Stream.concat(Stream.of(0),
lineBreak.matcher(source).results()
.map(MatchResult::end)).mapToInt(Integer::intValue).toArray();
src/jdk.internal.md/share/classes/jdk/internal/markdown/MarkdownTransformer.java
line 763:
> 761: *
> 762: * @param link the link node
> 763: */
Suggestion:
/**
* Visits a {@code Link} commonmark node.
*
* If the destination for the link begins with {@code code:}
* convert it to {@code {@link ...}} or {@code {@linkplain ...}} doc
tree node.
* {@code {@link ...}} will be used if the content (label) for
* the link is the same as the reference found after the {@code code:};
* otherwise, {@code {@linkplain ...}} will be used.
*
* The label will be left blank for {@code {@link ...}} nodes,
* implying that a default label should be used, based on the
* program element that was referenced.
*
* @param link the link node
*/
src/jdk.internal.md/share/classes/jdk/internal/markdown/MarkdownTransformer.java
line 778:
> 776: copyTo(getEndPos(link.getLastChild()));
> 777:
> 778: // determine whether to use {@link... } or
> {@linkplain ...}
Nit:
Suggestion:
// determine whether to use {@link ... } or {@linkplain ...}
src/jdk.internal.md/share/classes/jdk/internal/markdown/MarkdownTransformer.java
line 831:
> 829: /**
> 830: * {@return the position in the original comment for a position
> in {@code source},
> 831: * using {@link #replaceAdjustPos}}.
Suggestion:
* using {@link #replaceAdjustPos}}
src/jdk.internal.md/share/classes/jdk/internal/markdown/MarkdownTransformer.java
line 840:
> 838:
> 839: /**
> 840: * Process a node and any children.
Suggestion:
* Processes a node and any children.
src/jdk.internal.md/share/classes/jdk/internal/markdown/MarkdownTransformer.java
line 939:
> 937:
> 938: /**
> 939: * Flush any text in the {@code text} buffer, by creating a new
Suggestion:
* Flushes any text in the {@code text} buffer, by creating a new
src/jdk.internal.md/share/classes/jdk/internal/markdown/MarkdownTransformer.java
line 950:
> 948: }
> 949:
> 950:
Suggestion:
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1467870392
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1467870182
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1467643549
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1467871256
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1467876796
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1467871714
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1467872096
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1467872597
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v7]
On Fri, 19 Jan 2024 18:37:48 GMT, Jonathan Gibbons 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:
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v7]
On Tue, 23 Jan 2024 12:22:19 GMT, Pavel Rappo wrote:
>> 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 1388:
>
>> 1386: * @see > href="https://spec.commonmark.org/0.30/#thematic-breaks;>Thematic Break
>> 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 href="https://spec.commonmark.org/0.30/#thematic-breaks;>Thematic Break
> */
> THEMATIC_BREAK(Pattern.compile("((\*[ \t]*+){3,})|((-[
> \t]*+){3,})|((_[ \t]*+){3,})")),
To add to my earlier [comment], DocCommentParser recognises THEMATIC_BREAK
consisting of `-` as SETEXT_UNDERLINE. While it's inaccurate, it doesn't seem
important, as DCP's goal is to recognise and avoid Markdown, not process it.
[comment]: https://github.com/openjdk/jdk/pull/16388/files#r1462148038
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1466315132
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v7]
On Fri, 19 Jan 2024 18:37:48 GMT, Jonathan Gibbons 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/tree/DocTreeMaker.java line
238:
> 236: && postamble.isEmpty()
> 237: && fullBody.stream().anyMatch(t -> t.getKind()
> == Kind.MARKDOWN)
> 238: ? CommentStyle.JAVADOC_LINE :
> CommentStyle.JAVADOC_BLOCK;
While clever, it seems to be prone to false positive `JAVADOC_LINE`. Also, it
is inconsistent with `null` and `Position.NOPOS` returned from the `getText`
and `getSourcePos(int)` methods respectively.
src/jdk.compiler/share/classes/com/sun/tools/javac/tree/DocTreeMaker.java line
572:
> 570: }
> 571:
> 572: case TEXT -> {
I haven't looked at `SentenceBreaker` in detail, but one thing that bothers me
is that it sees a comment before that comment has been transformed. This means
that `///` comments might not "feel" like Markdown to authors.
src/jdk.compiler/share/classes/com/sun/tools/javac/tree/DocTreeMaker.java line
704:
> 702: }
> 703:
> 704: // If the break is well within the span of the string i.e.
> not
Oh irony! Sentence segmentation in javadoc has some problems with abbreviations
like that.
src/jdk.compiler/share/classes/com/sun/tools/javac/tree/DocTreeMaker.java line
790:
> 788:
> 789: // end of paragraph is newline, followed by a blank line or the
> beginning of the next block
> 790: private static final Pattern endPara = Pattern.compile("\n(([
> \t]*\n)|( {0,3}[-+*#=]))");
So DocTreeMaker now also knows about Markdown. I wonder if we can avoid that.
Also, I assume you mean this (`+` is not a part of "thematic break"):
Suggestion:
private static final Pattern endPara = Pattern.compile("\n(([ \t]*\n)|(
{0,3}[-_*#=]))");
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1464830924
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1465191542
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1465201174
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1465204965
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v7]
On Fri, 19 Jan 2024 18:37:48 GMT, Jonathan Gibbons 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 href="https://spec.commonmark.org/0.30/#setext-headings;>Setext Headings
> 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 href="https://spec.commonmark.org/0.30/#thematic-breaks;>Thematic Break
> 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 https://spec.commonmark.org/0.30/#thematic-breaks;>Thematic Break
*/
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 href="https://spec.commonmark.org/0.30/#code-fence;>Code Fence
> 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:
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v7]
On Fri, 19 Jan 2024 18:37:48 GMT, Jonathan Gibbons 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 271:
> 269: //if (textStart == -1) {
> 270: //textStart = bp;
> 271: //}
What's up with that?
src/jdk.compiler/share/classes/com/sun/tools/javac/parser/DocCommentParser.java
line 1333:
> 1331: lineKind = (ch == '\n' || ch == '\r') ?
> LineKind.BLANK
> 1332: : (indent <= 3) ? peekLineKind()
> 1333: : LineKind.OTHER;
Nested ternary-s are hard to read. Nested if-s are bulky. Sigh.
src/jdk.compiler/share/classes/com/sun/tools/javac/parser/DocCommentParser.java
line 1377:
> 1375: * @see href="https://spec.commonmark.org/0.30/#atx-headings;>ATX Headings
> 1376: */
> 1377: ATX_HEADER(Pattern.compile("#{1,6}( .*|$)")),
Actually, I wonder how accurate those regexes should match spec. Given the
definition [^*] of an ATX header and the fact that we always match the complete
(not find inside) a line, which by definition should not have line terminators,
shouldn't it be like this?
#{1,6}([ \t]+.*)?
[^*]: The opening sequence of # characters must be followed by spaces or tabs,
or by the end of line.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1462039425
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1462213698
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1462148038
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v7]
> 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 - Changes: https://git.openjdk.org/jdk/pull/16388/files Webrev: https://webrevs.openjdk.org/?repo=jdk=16388=06 Stats: 21334 lines in 191 files changed: 20679 ins; 266 del; 389 mod Patch: https://git.openjdk.org/jdk/pull/16388.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16388/head:pull/16388 PR: https://git.openjdk.org/jdk/pull/16388
