Re: RFR: 8333685: Make update-copyright-year script more useful
On Fri, 7 Jun 2024 19:01:45 GMT, Sonia Zaldana Calles wrote: > Hi all, > > This PR addresses [8333685](https://bugs.openjdk.org/browse/JDK-8333685). > > I have added the following enhancements: > - Removed uses of `fgrep` and `egrep` with `grep -F` and `grep -E` > respectively. > - Altered default behaviour to limit the processed changesets to those in the > current branch and the current year. > - Enabled an option to modify all changesets in the year if desired (this was > the previous default behaviour). > - Added named parameters and enabled `--help`. > - Removed mercurial support. > - Fixed a bug where copyright headers that didn't have a comma following the > initial year of creation were not getting picked up. For example, > [here](https://github.com/openjdk/jdk/blob/512b2b4f141f9a202984150b0427372e1a409a50/src/hotspot/cpu/zero/copy_zero.hpp#L3). > > - Fixed a bug where copyright headers missing the copyright symbol (c) were > not getting picked up. Refer to the example above as well. > > Thanks, > Sonia Opinion: while it's good to see improvements to the existent script, since JEP 330, we can now conveniently implement a similar script in Java. That'll also automatically take care of OS specifics. - PR Comment: https://git.openjdk.org/jdk/pull/19605#issuecomment-2160734896
Re: RFR: 8332545: Fix handling of HTML5 entities in Markdown comments
On Mon, 20 May 2024 20:17:10 GMT, Jonathan Gibbons wrote: > Please review a small fix to address a crash when handling HTML5 entities in > a Markdown doc comment. > > The path for the `entities.txt` (originally `entities.properties`) was not > correctly imported when importing the latest version of `commonmark-java`. > And, the makefiles need to be updated to include the `.txt` file in the > image. (The interim module for the interim javadoc already had this fix.) > > A simple new test is provided, containing entities that previously caused > `javadoc` to crash. Assuming the test fails without the fix, but passes with it, looks good. - Marked as reviewed by prappo (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19317#pullrequestreview-2067063183
Re: RFR: 8298405: Implement JEP 467: Markdown Documentation Comments [v69]
On Thu, 16 May 2024 14:53:17 GMT, Chen Liang wrote: > My rationale for a potential preview is that we changed > `-Xlint:dangling-doc-comments` as `///` is now dangling doc comment. Is this > considered a Java programming language change? There were some community > comments objecting the use of `///` for markdown documentation, and called > for alternative syntaxes like `/*markdown */`. It is certainly not a Java language change. The Java language and JLS have never bothered with the javadoc comments. If you are concerned with it being a lint warning rather than a **doc**lint warning, then it's a technicality: doclint sees less than lint sees, and sadly not enough for that check. Thus, that check was put in `lint. - PR Comment: https://git.openjdk.org/jdk/pull/16388#issuecomment-2115497564
Re: RFR: 8298405: Implement JEP 467: Markdown Documentation Comments [v69]
On Wed, 15 May 2024 21:04:36 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 incrementally with one > additional commit since the last revision: > > update tests for dangling doc comments, per review feedback > > A meta question about the JEP: Why don't Javadoc features (like snippets > > and markdown comments) don't go through preview, while Compiler features > > mostly do? Is there any bar for previews? > > Jon could probably reply more substantially, but my understanding is that > [_JEP 12: Preview Features_](https://openjdk.org/jeps/12) is barely > applicable here (emphasis mine): > > > ## Summary > > A _preview feature_ is a new feature of the Java language, Java Virtual > > Machine, or **Java SE API** that is fully specified, fully implemented, and > > yet impermanent. It is available in a JDK feature release to provoke > > developer feedback based on real world use; this may lead to it becoming > > permanent in a future Java SE Platform. > > Technically, the only reason we could invoke JEP 12 here would be a tiny > change to `Elements`, which is Java SE. But let's be honest, that change is > not what _JEP 467: Markdown Documentation Comments_ is about. Additionally, JavaDoc supports Preview APIs, but does not support previews (meta-previews?) of its own features. We simply don't have a mechanism to say: "Hey, that thing you are looking at is a new feature of JavaDoc. Click _here_ to learn more." - PR Comment: https://git.openjdk.org/jdk/pull/16388#issuecomment-2115404225
Re: RFR: 8298405: Implement JEP 467: Markdown Documentation Comments [v69]
On Thu, 16 May 2024 13:05:39 GMT, Chen Liang wrote: > A meta question about the JEP: Why don't Javadoc features (like snippets and > markdown comments) don't go through preview, while Compiler features mostly > do? Is there any bar for previews? Jon could probably reply more substantially, but my understanding is that [_JEP 12: Preview Features_](https://openjdk.org/jeps/12) is barely applicable here (emphasis mine): > ## Summary > > A *preview feature* is a new feature of the Java language, Java Virtual > Machine, or **Java SE API** that is fully specified, fully implemented, and > yet impermanent. It is available in a JDK feature release to provoke > developer feedback based on real world use; this may lead to it becoming > permanent in a future Java SE Platform. Technically, the only reason we could invoke JEP 12 here would be a tiny change to `Elements`, which is Java SE. But let's be honest, that change is not what _JEP 467: Markdown Documentation Comments_ is about. - PR Comment: https://git.openjdk.org/jdk/pull/16388#issuecomment-2115367104
Re: RFR: 8298405: Implement JEP 467: Markdown Documentation Comments [v69]
On Wed, 15 May 2024 21:04:36 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 incrementally with one > additional commit since the last revision: > > update tests for dangling doc comments, per review feedback It's probably the biggest update JavaDoc has seen in ages. Tremendous, good work. Thanks, Jon. - Marked as reviewed by prappo (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/16388#pullrequestreview-2060613760
Re: RFR: 8298405: Implement JEP 467: Markdown Documentation Comments [v67]
On Tue, 7 May 2024 17:31:29 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 91 commits: > > - Merge remote-tracking branch 'upstream/master' into > 8298405.doclet-markdown-v3 > - Remove `--no-fonts` from `MISSING_IN_MAN_PAGE` > - Update javadoc.1 troff man page > - Merge remote-tracking branch 'upstream/master' into > 8298405.doclet-markdown-v3 > - address review feedback, to improve testing of changes to Elements > - update copyright years > - Merge remote-tracking branch 'upstream/master' into > 8298405.doclet-markdown-v3 > - update commonmark-java from 0.21.0 to 0.22.0 > - Remove links to `jdk.javadoc` module from `java.compiler` module` > - Suppress warnings building tests > - ... and 81 more: https://git.openjdk.org/jdk/compare/524aaad9...cc12140a I think we should add a test to verify that if `--disable-line-doc-comments` is specified, no `///` dangling comments are reported. - PR Comment: https://git.openjdk.org/jdk/pull/16388#issuecomment-2112100605
Re: RFR: JDK-8298405: Implement JEP 467: Markdown Documentation Comments [v55]
On Mon, 8 Apr 2024 21:12:42 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 incrementally with one
> additional commit since the last revision:
>
> add support for JDK-8329296: Update Elements for '///' documentation
> comments
Changes for 21f5b00 mostly look good, but I'm not a compiler person.
src/jdk.compiler/share/classes/com/sun/tools/javac/model/JavacElements.java
line 443:
> 441: @DefinedBy(Api.LANGUAGE_MODEL)
> 442: public CommentKind getDocCommentKind(Element e) {
> 443: return getDocCommentItem(e, ((docCommentTable, tree) ->
> docCommentTable.getCommentKind(tree)));
Nit:
Suggestion:
return getDocCommentItem(e, ((docCommentTable, tree) ->
docCommentTable.getCommentKind(tree)));
src/jdk.compiler/share/classes/com/sun/tools/javac/model/JavacElements.java
line 443:
> 441: @DefinedBy(Api.LANGUAGE_MODEL)
> 442: public CommentKind getDocCommentKind(Element e) {
> 443: return getDocCommentItem(e, ((docCommentTable, tree) ->
> docCommentTable.getCommentKind(tree)));
Again:
Suggestion:
return getDocCommentItem(e, ((docCommentTable, tree) ->
docCommentTable.getCommentKind(tree)));
src/jdk.compiler/share/classes/com/sun/tools/javac/tree/DocCommentTable.java
line 55:
> 53:
> 54: /**
> 55: * Get the plain text of the doc comment, if any, for a tree node.
This is likely a copy-pasted comment.
-
PR Review: https://git.openjdk.org/jdk/pull/16388#pullrequestreview-1988489764
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1557248565
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1557249290
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1557444619
Re: RFR: JDK-8303689: javac -Xlint could/should report on "dangling" doc comments
On Wed, 27 Mar 2024 22:04:30 GMT, Jonathan Gibbons wrote:
> Please review the updates to support a proposed new
> `-Xlint:dangling-doc-comments` option.
>
> The work can be thought of as in 3 parts:
>
> 1. An update to the `javac` internal class `DeferredLintHandler` so that it
> is possible to specify the appropriately configured `Lint` object when it is
> time to consider whether to generate the diagnostic or not.
>
> 2. Updates to the `javac` front end to record "dangling docs comments" found
> near the beginning of a declaration, and to report them using an instance of
> `DeferredLintHandler`. This allows the warnings to be enabled or disabled
> using the standard mechanisms for `-Xlint` and `@SuppressWarnings`. The
> procedure for handling dangling doc comments is described in this comment in
> `JavacParser`.
>
> * Dangling documentation comments are handled as follows.
> * 1. {@code Scanner} adds all doc comments to a queue of
> * recent doc comments. The queue is flushed whenever
> * it is known that the recent doc comments should be
> * ignored and should not cause any warnings.
> * 2. The primary documentation comment is the one obtained
> * from the first token of any declaration.
> * (using {@code token.getDocComment()}.
> * 3. At the end of the "signature" of the declaration
> * (that is, before any initialization or body for the
> * declaration) any other "recent" comments are saved
> * in a map using the primary comment as a key,
> * using this method, {@code saveDanglingComments}.
> * 4. When the tree node for the declaration is finally
> * available, and the primary comment, if any,
> * is "attached", (in {@link #attach}) any related
> * dangling comments are also attached to the tree node
> * by registering them using the {@link #deferredLintHandler}.
> * 5. (Later) Warnings may be genereated for the dangling
> * comments, subject to the {@code -Xlint} and
> * {@code @SuppressWarnings}.
>
>
> 3. Updates to the make files to disable the warnings in modules for which
> the
> warning is generated. This is often because of the confusing use of `/**` to
> create box or other standout comments.
src/jdk.javadoc/share/man/javadoc.1 line 111:
> 109: source code with the \f[V]javac\f[R] option \f[V]-Xlint\f[R], or more
> 110: specifically, \f[V]-Xlint:dangling-doc-comments\f[R].
> 111: Within a source file, you may use suppress any warnings generated by
Typo?
Suggestion:
Within a source file, you may suppress any warnings generated by
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18527#discussion_r1542131487
Re: RFR: JDK-8303689: javac -Xlint could/should report on "dangling" doc comments
On Wed, 27 Mar 2024 22:04:30 GMT, Jonathan Gibbons wrote:
> Please review the updates to support a proposed new
> `-Xlint:dangling-doc-comments` option.
>
> The work can be thought of as in 3 parts:
>
> 1. An update to the `javac` internal class `DeferredLintHandler` so that it
> is possible to specify the appropriately configured `Lint` object when it is
> time to consider whether to generate the diagnostic or not.
>
> 2. Updates to the `javac` front end to record "dangling docs comments" found
> near the beginning of a declaration, and to report them using an instance of
> `DeferredLintHandler`. This allows the warnings to be enabled or disabled
> using the standard mechanisms for `-Xlint` and `@SuppressWarnings`. The
> procedure for handling dangling doc comments is described in this comment in
> `JavacParser`.
>
> * Dangling documentation comments are handled as follows.
> * 1. {@code Scanner} adds all doc comments to a queue of
> * recent doc comments. The queue is flushed whenever
> * it is known that the recent doc comments should be
> * ignored and should not cause any warnings.
> * 2. The primary documentation comment is the one obtained
> * from the first token of any declaration.
> * (using {@code token.getDocComment()}.
> * 3. At the end of the "signature" of the declaration
> * (that is, before any initialization or body for the
> * declaration) any other "recent" comments are saved
> * in a map using the primary comment as a key,
> * using this method, {@code saveDanglingComments}.
> * 4. When the tree node for the declaration is finally
> * available, and the primary comment, if any,
> * is "attached", (in {@link #attach}) any related
> * dangling comments are also attached to the tree node
> * by registering them using the {@link #deferredLintHandler}.
> * 5. (Later) Warnings may be genereated for the dangling
> * comments, subject to the {@code -Xlint} and
> * {@code @SuppressWarnings}.
>
>
> 3. Updates to the make files to disable the warnings in modules for which
> the
> warning is generated. This is often because of the confusing use of `/**` to
> create box or other standout comments.
Javadoc changes look trivially good. I only note that the javadoc man page diff
contains some unrelated changes.
-
PR Comment: https://git.openjdk.org/jdk/pull/18527#issuecomment-2024116236
Re: RFR: JDK-8303689: javac -Xlint could/should report on "dangling" doc comments
On Wed, 27 Mar 2024 22:04:30 GMT, Jonathan Gibbons wrote:
> Please review the updates to support a proposed new
> `-Xlint:dangling-doc-comments` option.
>
> The work can be thought of as in 3 parts:
>
> 1. An update to the `javac` internal class `DeferredLintHandler` so that it
> is possible to specify the appropriately configured `Lint` object when it is
> time to consider whether to generate the diagnostic or not.
>
> 2. Updates to the `javac` front end to record "dangling docs comments" found
> near the beginning of a declaration, and to report them using an instance of
> `DeferredLintHandler`. This allows the warnings to be enabled or disabled
> using the standard mechanisms for `-Xlint` and `@SuppressWarnings`. The
> procedure for handling dangling doc comments is described in this comment in
> `JavacParser`.
>
> * Dangling documentation comments are handled as follows.
> * 1. {@code Scanner} adds all doc comments to a queue of
> * recent doc comments. The queue is flushed whenever
> * it is known that the recent doc comments should be
> * ignored and should not cause any warnings.
> * 2. The primary documentation comment is the one obtained
> * from the first token of any declaration.
> * (using {@code token.getDocComment()}.
> * 3. At the end of the "signature" of the declaration
> * (that is, before any initialization or body for the
> * declaration) any other "recent" comments are saved
> * in a map using the primary comment as a key,
> * using this method, {@code saveDanglingComments}.
> * 4. When the tree node for the declaration is finally
> * available, and the primary comment, if any,
> * is "attached", (in {@link #attach}) any related
> * dangling comments are also attached to the tree node
> * by registering them using the {@link #deferredLintHandler}.
> * 5. (Later) Warnings may be genereated for the dangling
> * comments, subject to the {@code -Xlint} and
> * {@code @SuppressWarnings}.
>
>
> 3. Updates to the make files to disable the warnings in modules for which
> the
> warning is generated. This is often because of the confusing use of `/**` to
> create box or other standout comments.
Would this be the first lint -- not doclint -- warning related to comments, let
alone doc comments?
-
PR Comment: https://git.openjdk.org/jdk/pull/18527#issuecomment-2024106466
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v44]
On Mon, 4 Mar 2024 14:40:06 GMT, Pavel Rappo wrote:
> I have a test case to report. The following results in no `@param`
> information being rendered, which I think is a bug:
>
> ```
> /// Hello, _Markdown_ world!
> ///
> ///
> /// @param hello
> ///
> ///
> public class C { }
> ```
Scratch that. After experimenting a bit more with **traditional** javadoc
comments, I realised that it might be expected that `@param` would be lost in
that `...` block; okay.
Still, I find it surprising that the description of the `@param` tag in the
above comment, hosted in `///` or `/**...*/`, is a single node of type
`RawText` with the following content:
hello
(Note, the content includes ``.)
-
PR Comment: https://git.openjdk.org/jdk/pull/16388#issuecomment-1976818803
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v44]
On Fri, 1 Mar 2024 21:49:29 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 incrementally with one
> additional commit since the last revision:
>
> Revise `Markdown.update` to better handle container blocks.
I have a test case to report. The following results in no `@param` information
being rendered, which I think is a bug:
/// Hello, _Markdown_ world!
///
///
/// @param hello
///
///
public class C { }
-
PR Comment: https://git.openjdk.org/jdk/pull/16388#issuecomment-1976734010
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v36]
On Fri, 16 Feb 2024 07:42:47 GMT, Hannes Wallnöfer wrote:
>> Jonathan Gibbons has updated the pull request incrementally with two
>> additional commits since the last revision:
>>
>> - Support for Table Of Contents in Markdown headings
>> - fix typo
>
> src/jdk.compiler/share/classes/com/sun/tools/javac/parser/DocCommentParser.java
> line 286:
>
>> 284: lineKind = (ch == '\n' || ch == '\r') ?
>> LineKind.BLANK
>> 285: : (indent <= 3) ? peekLineKind()
>> 286: : lineKind != LineKind.OTHER ?
>> LineKind.INDENTED_CODE_BLOCK
>
> Doesn't this cause false positives for indented code blocks? In my
> understanding, indented lines [in a list
> context](https://spec.commonmark.org/0.30/#example-258) and [directly
> following a
> blockquote](https://spec.commonmark.org/dingus/?text=%3E%20%20%20%20foo%0A%20%20%20%20%20bar%0A)
> are not interpreted as code blocks, but as part of the list or blockquote.
> So my guess would be that `not OTHER` should be something like `BLANK or
> INDENTED_CODE_BLOCK or HEADER`, and that still leaves the problem of a [blank
> line in a list context](https://spec.commonmark.org/0.30/#example-108).
Sigh. I remember when we all hoped that we wouldn't need to go in the weeds of
Markdown parsing, but here we are. Hannes is right. Here are a coupe of
problematic examples:
- /// List
///
/// - item
///
/// @param
- /// > Quote
/// >
/// > {@link java.lang.Object}
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1492430822
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v32]
On Thu, 15 Feb 2024 19:39:07 GMT, Pavel Rappo wrote:
>> whitespace is handled separately, on line 280 (`readIndent`) and285 (`:
>> (indent <= 3) ? peekLineKind()`)
>
> Correct, but I believe the ordered list marker should be like this:
>
> ORDERED_LIST_ITEM(Pattern.compile("[0-9]{1,9}[.)] .*"))
> ^
>
> Note extra whitespace character. I think we should really add this to our
> tests, since lists are foundational Markdown structures, probably right after
> italics and bold.
Then we should add `[ \t]` to both constants, right:
BULLETED_LIST_ITEM(Pattern.compile("[-+*][ \t].*"))
ORDERED_LIST_ITEM(Pattern.compile("[0-9]{1,9}[.)][ \t].*"))
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1491564839
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v32]
On Thu, 15 Feb 2024 19:20:23 GMT, Jonathan Gibbons wrote:
>> src/jdk.compiler/share/classes/com/sun/tools/javac/parser/DocCommentParser.java
>> line 1401:
>>
>>> 1399: */
>>> 1400: enum LineKind {
>>> 1401: BLANK(Pattern.compile("[ \t]*")),
>>
>> `BLANK` is a pseudo kind, because it is set manually, but never returned
>> from `peekLine()`. I wonder if we can change it somehow.
>
> Even if it is set manually, it is still appropriate to have it as a member in
> the `LineKind` enum class.
Not that it invalidates your reply; clarification: I meant `peekLineKind()`,
not `peekLine()`.
>> src/jdk.compiler/share/classes/com/sun/tools/javac/parser/DocCommentParser.java
>> line 1433:
>>
>>> 1431: * @see >> href="https://spec.commonmark.org/0.30/#list-items;>List items
>>> 1432: */
>>> 1433: BULLETED_LIST_ITEM(Pattern.compile("[-+*] .*")),
>>
>> This comment is about `BULLETED_LIST_ITEM` and `ORDERED_LIST_ITEM`
>> constants. I know that we don't need to be very precise, but perhaps in this
>> case we should. While the CommonMark spec is a vague on that, from my
>> experiments with [dingus](https://spec.commonmark.org/dingus/), it appears
>> that a list marker can be preceded and followed by some number of whitespace
>> characters.
>
> whitespace is handled separately, on line 280 (`readIndent`) and285 (`:
> (indent <= 3) ? peekLineKind()`)
Correct, but I believe the ordered list marker should be like this:
ORDERED_LIST_ITEM(Pattern.compile("[0-9]{1,9}[.)] .*"))
^
Note extra whitespace character. I think we should really add this to our
tests, since lists are foundational Markdown structures, probably right after
italics and bold.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1491550784
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1491545609
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v32]
On Thu, 15 Feb 2024 00:30:25 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 44 commits:
>
> - fill in `visitRawText` in `CommentHelper.getTags` visitor
> - fixes for the "New API" page
> - change "standard" to "traditional" when referring to a comment
> - Merge remote-tracking branch 'upstream/master' into
> 8298405.doclet-markdown-v3
> - Merge remote-tracking branch 'upstream/master' into
> 8298405.doclet-markdown-v3
> - improve support for DocCommentParser.LineKind
> - Merge remote-tracking branch 'upstream/master' into
> 8298405.doclet-markdown-v3 # Please enter a commit message to explain why
> this merge is necessary, # especially if it merges an updated upstream into a
> topic branch. # # Lines starting with '#' will be ignored, and an empty
> message aborts # the
>commit.
> - update copyright year on test
> - refactor recent new test case in TestMarkdown.java
> - address review feedback
> - ... and 34 more: https://git.openjdk.org/jdk/compare/8765b176...2801c2e1
I'm again looking `LineKind`. This time because of 9eaf84e5dd6.
src/jdk.compiler/share/classes/com/sun/tools/javac/parser/DocCommentParser.java
line 422:
> 420: defaultContentCharacter();
> 421: }
> 422: }
Is it to pass `` through to Markdown, to allow it to deal with Markdown escapes?
src/jdk.compiler/share/classes/com/sun/tools/javac/parser/DocCommentParser.java
line 1401:
> 1399: */
> 1400: enum LineKind {
> 1401: BLANK(Pattern.compile("[ \t]*")),
`BLANK` is a pseudo kind, because it is set manually, but never returned from
`peekLine()`. I wonder if we can change it somehow.
src/jdk.compiler/share/classes/com/sun/tools/javac/parser/DocCommentParser.java
line 1433:
> 1431: * @see href="https://spec.commonmark.org/0.30/#list-items;>List items
> 1432: */
> 1433: BULLETED_LIST_ITEM(Pattern.compile("[-+*] .*")),
This comment is about `BULLETED_LIST_ITEM` and `ORDERED_LIST_ITEM` constants. I
know that we don't need to be very precise, but perhaps in this case we should.
While the CommonMark spec is a vague on that, from my experiments with
[dingus](https://spec.commonmark.org/dingus/), it appears that a list marker
can be preceded and followed by some number of whitespace characters.
-
PR Review: https://git.openjdk.org/jdk/pull/16388#pullrequestreview-1883374712
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1491362821
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1491344667
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1491354450
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v31]
On Mon, 12 Feb 2024 23:52:35 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 40 commits:
>
> - Merge remote-tracking branch 'upstream/master' into
> 8298405.doclet-markdown-v3
> - improve support for DocCommentParser.LineKind
> - Merge remote-tracking branch 'upstream/master' into
> 8298405.doclet-markdown-v3 # Please enter a commit message to explain why
> this merge is necessary, # especially if it merges an updated upstream into a
> topic branch. # # Lines starting with '#' will be ignored, and an empty
> message aborts # the
>commit.
> - update copyright year on test
> - refactor recent new test case in TestMarkdown.java
> - address review feedback
> - address review feedback
> - added test case in TestMarkdown.java for handling of `@deprecated` tag
> - amend comment in test
> - improve comments on negative test case
> - ... and 30 more: https://git.openjdk.org/jdk/compare/2ed889b7...1c64a6e0
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/HtmlDocletWriter.java
line 563:
> 561:
> 562: /**
> 563: * {@returns the plain-text content of a named HTML element in a
> list of content}
Suggestion:
* {@return the plain-text content of a named HTML element in a list of
content}
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclint/Checker.java line
1021:
> 1019: .findFirst();
> 1020: if (first.isEmpty() || first.get() != tree) {
> 1021: dct.getFirstSentence().forEach(t ->
> System.err.println(t.getKind() + ": >>|" + t + "|<<"));
Is it leftover debug output?
src/jdk.javadoc/share/classes/jdk/javadoc/internal/tool/Start.java line 571:
> 569: // of a doclet to be specified instead of the name of the
> 570: // doclet class and optional doclet path.
> 571: // See https://bugs.openjdk.org/browse/JDK-8263219
It's hard to understand this:
> to permit an instance of an appropriately configured instance of a doclet
Also: how is that piece of code used? When I commented it out, no
test/langtools:langtools_javadoc tests failed.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1487629102
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1487659843
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1487642764
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v30]
On Fri, 9 Feb 2024 22:17:43 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 incrementally with one
> additional commit since the last revision:
>
> refactor recent new test case in TestMarkdown.java
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/HtmlDocletWriter.java
line 1465:
> 1463: markdownInput.append('\n');
> 1464: }
> 1465: markdownInput.append(PLACEHOLDER_BLOCK);
There's quite a lot of overlap with the recently simplified
https://github.com/openjdk/jdk/blob/7c6316886d1ae86a663d996dae373c42281622fd/src/jdk.internal.md/share/classes/jdk/internal/markdown/MarkdownTransformer.java#L220-L238
If it's impractical to factor out the common bits, maybe we could at least make
the respective parts of HtmlDocletWriter as close as possible to those of
MarkdownTransformer.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1486548869
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v22]
On Mon, 12 Feb 2024 16:29:19 GMT, Jonathan Gibbons wrote: > I guess it depends how much we want to import the code as-is, without knowing > any lint-warts. I think it would be nice not to have to modify code beyond superficial edits: addition of headers, renaming of packages, and converting of non-ASCII symbols. - PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1486526603
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 [v28]
On Thu, 8 Feb 2024 15:38:26 GMT, Pavel Rappo wrote: >> test/langtools/jdk/javadoc/tool/sampleapi/lib/sampleapi/generator/ModuleGenerator.java >> line 49: >> >>> 47: import sampleapi.util.PoorDocCommentTable; >>> 48: >>> 49: import static >>> com.sun.tools.javac.parser.Tokens.Comment.CommentStyle.JAVADOC; >> >> To clarify, the change to this file is that you removed two unused imports, >> right? > > Ping. Maybe if it is not essential, we could remove that change, to keep PR > more focused. Okay I can see that you tried to revert the change to that file and published a [PR] to optimise imports separately. Sadly, the "revert" introduced even more churn. Here's my proposal, which might not be elegant but does the job: git checkout 8298405.doclet-markdown-v3 git diff -R head --not upstream/master -- test/langtools/jdk/javadoc/tool/sampleapi/lib/sampleapi/generator/ModuleGenerator.java | git apply git commit -am "Revert all changes to ModuleGenerator.java" If you have any issues with that snippet, ping me out of band; I hope the intent of the snippet is clear though. [PR]: https://github.com/openjdk/jdk/pull/17782 - PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1484477033
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v28]
On Thu, 8 Feb 2024 21:46:34 GMT, Jonathan Gibbons wrote:
>> I believe this is substantially covered in line 226-227. See the third call
>> to `test` in the following group of lines:
>>
>>
>> for (String src : sources) {
>> test(src);
>> test(src.replaceAll("@Deprecated", "/** @deprecated */"));
>> test(src.replaceAll("deprecated", "notDeprecated2") //
>> change class name
>> .replaceAll("@Deprecated", "/// @deprecated\n"));
>> }
>>
>>
>> * The first call, `test(src)`, runs all the test cases, using the
>> `@Deprecated` annotation by default. In these test cases, the name of the
>> element encodes whether it is expected that the element is deprecated or not.
>>
>> * The second call, `test(src.replaceAll("deprecated", "notDeprecated2")`,
>> runs the test cases again, after changing the annotation to a traditional
>> (`/** ...*/`) comment containing the `@deprecated` tag. This is a
>> long-standing call, and tests the legacy behavior of `@deprecated` appearing
>> in a traditional doc comment. Effectively, it tests whether a `/**
>> @deprecated */` comment has equivalent behavior to a `@Deprecated` comment.
>>
>> * The third call is new to this PR and the functionality to support Markdown
>> comments. It makes two changes to the source for the test cases, before
>> running them:
>>1. as in the previous test, the annotations are replaced by a comment
>> containing `@deprecated` -- except that this time, the comment is a
>> new-style `/// ... ` comment instead of a traditional `/** ... */` comment,
>> and ...
>>2. because we do not expect `/// @deprecated` to indicate deprecation, we
>> need to change the expected result for each test case, which we do by
>> changing the element name for the test case. The change is the first call
>> to replace to avoid false positives after changing the doc comment. The
>> change uses a new name, `notDeprecated2`, in which `notDeprecated` encodes
>> the expected deprecation status, and the `2` is to differentiate the
>> declarations from other declarations in the case case that already use the
>> name `notDeprecated`.
>
> While the underlying mechanism in javac for indicating deprecation is the
> same for all, I accept this is primarily a test for generated class files. I
> can add a javadoc test for basic behavior of `@deprecated` in Markdown
> comments.
Thanks for confirming that there's a test to check that `/// @deprecated` is a
no-op and for patiently explaining how that test works.
I confirm that
test/langtools/tools/javac/classfiles/attributes/deprecated/DeprecatedTest.java
starts failing if I introduce the following change:
---
a/src/jdk.compiler/share/classes/com/sun/tools/javac/parser/JavaTokenizer.java
+++
b/src/jdk.compiler/share/classes/com/sun/tools/javac/parser/JavaTokenizer.java
@@ -1638,7 +1638,7 @@ protected void scanDocComment() {
? trimJavadocLineComment(line, indent)
: trimJavadocComment(line);
-if (cs == CommentStyle.JAVADOC_BLOCK) {
+//if (cs == CommentStyle.JAVADOC_BLOCK) {
// If standalone @deprecated tag
int pos = line.position();
line.skipWhitespace();
@@ -1652,7 +1652,7 @@ protected void scanDocComment() {
}
line.reset(pos);
-}
+//}
putLine(line);
}
I can also see that you have introduced a dedicated
test/langtools/jdk/javadoc/doclet/testMarkdown/TestMarkdown.java#testDeprecated
which also starts failing with that change of mine.
Please update the copyright year in
test/langtools/jdk/javadoc/doclet/testMarkdown/TestMarkdown.java.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1484410402
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 [v28]
On Wed, 15 Nov 2023 00:37:06 GMT, Jonathan Gibbons wrote:
>> test/langtools/jdk/javadoc/tool/testTransformer/TestTransformer.java line 96:
>>
>>> 94: var sl =
>>> ServiceLoader.load(DocTrees.DocCommentTreeTransformer.class);
>>> 95: return StreamSupport.stream(sl.spliterator(), false)
>>> 96: .filter(p -> p.name().equals(name))
>>
>> ServiceLoader specification suggests another way of streaming:
>> Suggestion:
>>
>> return sl.stream()
>> .map(ServiceLoader.Provider::get)
>> .filter(t -> t.name().equals(name))
>>
>> Would it be the same and more readable?
>
> Hmm. It's longer, but arguably simpler to comprehend than using a
> spliterator. I've changed the code.
There's now a leftover `import java.util.stream.StreamSupport;` in that file.
>> test/langtools/tools/javac/doctree/MDPrinter.java line 67:
>>
>>> 65: * Conceptually based on javac's {@code DPrinter}.
>>> 66: */
>>> 67: public class MDPrinter {
>>
>> While DPrinter is used, MDPrinter isn't. (At least, I could't find any
>> usages of it.) If you feel like MDPrinter is important, we should at least
>> add a compile test for it, to protect it from bitrot.
>
>
MDPrinter.java is now a test that compiles itself; thanks.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1484541756
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1484196535
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v28]
On Thu, 8 Feb 2024 15:35:58 GMT, Pavel Rappo wrote: >> Please update the DocComment printout in that commented out test: the actual >> content is different. It would be nice if the test were passing at least at >> the moment of its initial commit. >> >> Here's what I see locally: >> >> >> Expect: >> DocComment[DOC_COMMENT, pos:0 >> firstSentence: 1 >> Summary[SUMMARY, pos:4 >> summary: 1 >> Erroneous[ERRONEOUS, pos:14, prefPos:37 >> code: compiler.err.dc.unterminated.inline.tag >> body: abc_`|_def}|_rest_`more` >> ] >> ] >> body: empty >> block tags: empty >> ] >> >> Found: >> DocComment[DOC_COMMENT, pos:0 >> firstSentence: 1 >> Summary[SUMMARY, pos:1 >> summary: 1 >> Erroneous[ERRONEOUS, pos:11, prefPos:32 >> code: compiler.err.dc.unterminated.inline.tag >> body: abc_`|def}|rest_`more` >> ] >> ] >> body: empty >> block tags: empty >> ] > > Ping. Thanks for fixing the test and clarifying its comment. - PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1484174598
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v6]
On Mon, 29 Jan 2024 23:50:25 GMT, Jonathan Gibbons wrote: >> Probably, yes. > > New class comment added. I can see it and it reads nicely; thanks! - PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1484586216
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v6]
On Tue, 30 Jan 2024 00:47:33 GMT, Jonathan Gibbons wrote: > > Musing on this more. > > Can/should we, without introducing probably unwelcome `Kind.MD` to > > `javax.tools.JavaFileObject.Kind`, teach javac to recognise `package.md` > > similarly to how it recognises legacy `package.html`? If we are aiming for > > Markdown to be a drop in replacement for traditional javadoc comments, we > > might want to go the extra mile. > > I'm pleased to see that Markdown `-overview` files work just fine. > > No. There are times to let go of legacy behavior, and even if this is not the > time to remove support for `package.html`, there is no reason to go backwards > and support `package.md`. The preferred replacement for `package.html` has > long been `package-info.java` and you can put Markdown content in that file > with no issues. > > In similar fashion, remember the recent discussion as to whether we should > support `@deprecated` in Markdown comments as marking the declaration as > _deprecated_, even without the `@Deprecated` annotation. The general > consensus was to not persist with that legacy behavior. Okay. - PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1484693923
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v6]
On Mon, 29 Jan 2024 23:34:24 GMT, Jonathan Gibbons wrote:
>> src/jdk.compiler/share/classes/com/sun/tools/javac/parser/DocCommentParser.java
>> line 215:
>>
>>> 213: case '\n', '\r' -> {
>>> 214: return newString(bp, p);
>>> 215: }
>>
>> Hm... this does not seem to be consistent with `newline` in `nextChar`;
>> should it be consistent?
>
> I think it is OK, isn't it?
>
> In both cases, a newline sequence can begin with `\r` or `\n`.
>
> In this `peekLine` method, we only want the content up to but not including
> the newline, so there is no need to handle the possibility of `\r\n`. In
> `nextChar`, we do want to detect `r` and `\r\n` and treat both as equivalent
> to `\n`.
>
> Or am I missing something?
You don't seem to be missing anything; it's me who was confused.
>> src/jdk.compiler/share/classes/com/sun/tools/javac/parser/DocCommentParser.java
>> line 1295:
>>
>>> 1293: switch (ch) {
>>> 1294: case ' ' -> indent++;
>>> 1295: case '\t' -> indent = 4;
>>
>> Shouldn't it be like this or it does not matter?
>> ```suggestion
>> case '\t' -> indent += 4;
>
> I did mean `indent = 4`.
> It would not mean `indent +=4` because you would have to take preceding
> character count into account, to round up to a multiple of 4.
> But anyway, it's enough to know the indent is 4 or more, meaning the code is
> looking at an indented code block.
> https://spec.commonmark.org/0.30/#indented-code-blocks
I'm not sure I understood the _take preceding character count into account_
part, but if `indent = 4` is what you meant and having read that section of the
spec, I'm okay with it. Thanks.
>> src/jdk.compiler/share/classes/com/sun/tools/javac/parser/DocCommentParser.java
>> line 1421:
>>
>>> 1419: case '#', '=', '-', '+', '_', '`', '~' -> {
>>> 1420: String line = peekLine();
>>> 1421: for (LineKind lk : LineKind.values()) {
>>
>> Nothing wrong here, just noting that this is one more way one can depend on
>> an enum constant order. Change it, and a peeked line kind might change too
>> (e.g. `OTHER` matches everything.)
>
> Like it or not, JLS defines that enum members are ordered, and even has an
> example 8.9.3-1 of using the `values` method in an enhanced `for` loop. Any
> change to the order of the members of any enum has the potential to be a
> breaking change and should never be done lightly. Curiously, JLS 13.4.26
> does not say that reordering enum constants may break clients.
>
> Anyway, I added comments to the LineKind enum declaration.
I think I'm still coming to terms with this feature of enum constants: being
order-sensitive.
https://docs.oracle.com/javase/specs/jls/se21/html/jls-13.html#jls-13.4.26 is
concerned with "binary compatibility". What we are talking about here is more
like "behavioural compatibilty" as defined in
https://wiki.openjdk.org/display/csr/Kinds+of+Compatibility. But we digress.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1484581568
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1484593858
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1484607076
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v22]
On Wed, 7 Feb 2024 18:14:24 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 28 commits:
>
> - Merge remote-tracking branch 'upstream/master' into
> 8298405.doclet-markdown-v3 # Please enter a commit message to explain why
> this merge is necessary, # especially if it
>merges an updated upstream into a topic branch. # # Lines starting with
> '#' will be ignored, and an empty message aborts # the commit.
> - add test case contributed by @lahodaj
> - initial fix for source offset problem
> - remove unused imports
> - First pass at remove DocCommentTransformer from the public API.
>
>It is still declared internally, and installed by default, using the
> service-provider mechanism.
>If the standard impl is not available, an identity transformer is used.
> - updates for "first sentence" issues and tests
> - add info about provenance of `jdk.internal.md` module
> - MarkdownTransformer: tweak doc comments
> - MarkdownTransformer: change `Lower.replaceIter` to be `private final`
> - MarkdownTransformer: use suggested text for using streams
> - ... and 18 more: https://git.openjdk.org/jdk/compare/18e24d06...63dd8bf4
src/jdk.internal.md/share/classes/module-info.java line 41:
> 39: // * package and import statements are updated
> 40: // * characters outside the ASCII range are converted to Unicode escapes
> 41: // * @SuppressWarnings("fallthrough") is added to getSetextHeadingLevel
I wonder if it would be better to compile this module without (some) lint
checks. Is it possible?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1483335356
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 [v22]
On Thu, 11 Jan 2024 15:07:33 GMT, Pavel Rappo wrote: >> test/langtools/tools/javac/classfiles/attributes/deprecated/DeprecatedTest.java >> line 26: >> >>> 24: /* >>> 25: * @test >>> 26: * @bug 8042261 8298405 >> >> This comment is not for this line, but for two compiler tests for >> `@deprecated` javadoc tag. It seemed quite contextual place to put it. >> >> Did I miss it, or you are planning to add a javadoc test that verifies that >> `@deprecated` appearing in a `///` comment has no [special meaning] it has >> in classic `/** */` comments? >> >> [special meaning]: >> https://github.com/openjdk/jdk/blob/128363bf3b57dfa05b3807271b47851733c1afb9/src/jdk.compiler/share/classes/com/sun/tools/javac/parser/JavaTokenizer.java#L1639-L1653 > > Ping. I do believe that it's important to have such a test. Ping. >> test/langtools/tools/javac/doctree/DocCommentTester.java line 1012: >> >>> 1010: //} >>> 1011: //return null; >>> 1012: //} >> >> Debugging leftover? > > If you want to leave it for debugging you can make it private and uncomment. Ping. >> test/langtools/tools/javac/doctree/MarkdownTest.java line 555: >> >>> 553: // block tags: empty >>> 554: //] >>> 555: //*/ >> >> Just to clarify: it is supposed to be commented out, right? If uncommented, >> this test fails with a slightly different error. > > Please update the DocComment printout in that commented out test: the actual > content is different. It would be nice if the test were passing at least at > the moment of its initial commit. > > Here's what I see locally: > > > Expect: > DocComment[DOC_COMMENT, pos:0 > firstSentence: 1 > Summary[SUMMARY, pos:4 > summary: 1 > Erroneous[ERRONEOUS, pos:14, prefPos:37 > code: compiler.err.dc.unterminated.inline.tag > body: abc_`|_def}|_rest_`more` > ] > ] > body: empty > block tags: empty > ] > > Found: > DocComment[DOC_COMMENT, pos:0 > firstSentence: 1 > Summary[SUMMARY, pos:1 > summary: 1 > Erroneous[ERRONEOUS, pos:11, prefPos:32 > code: compiler.err.dc.unterminated.inline.tag > body: abc_`|def}|rest_`more` > ] > ] > body: empty > block tags: empty > ] Ping. - PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1483179667 PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1483179302 PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1483178073
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v22]
On Wed, 8 Nov 2023 17:17: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 28 commits: >> >> - Merge remote-tracking branch 'upstream/master' into >> 8298405.doclet-markdown-v3 # Please enter a commit message to explain why >> this merge is necessary, # especially if it >>merges an updated upstream into a topic branch. # # Lines starting with >> '#' will be ignored, and an empty message aborts # the commit. >> - add test case contributed by @lahodaj >> - initial fix for source offset problem >> - remove unused imports >> - First pass at remove DocCommentTransformer from the public API. >> >>It is still declared internally, and installed by default, using the >> service-provider mechanism. >>If the standard impl is not available, an identity transformer is used. >> - updates for "first sentence" issues and tests >> - add info about provenance of `jdk.internal.md` module >> - MarkdownTransformer: tweak doc comments >> - MarkdownTransformer: change `Lower.replaceIter` to be `private final` >> - MarkdownTransformer: use suggested text for using streams >> - ... and 18 more: https://git.openjdk.org/jdk/compare/18e24d06...63dd8bf4 > > test/langtools/jdk/javadoc/tool/sampleapi/lib/sampleapi/generator/ModuleGenerator.java > line 49: > >> 47: import sampleapi.util.PoorDocCommentTable; >> 48: >> 49: import static >> com.sun.tools.javac.parser.Tokens.Comment.CommentStyle.JAVADOC; > > To clarify, the change to this file is that you removed two unused imports, > right? Ping. Maybe if it is not essential, we could remove that change, to keep PR more focused. - PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1483182361
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v22]
On Wed, 7 Feb 2024 18:14:24 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 28 commits: > > - Merge remote-tracking branch 'upstream/master' into > 8298405.doclet-markdown-v3 # Please enter a commit message to explain why > this merge is necessary, # especially if it >merges an updated upstream into a topic branch. # # Lines starting with > '#' will be ignored, and an empty message aborts # the commit. > - add test case contributed by @lahodaj > - initial fix for source offset problem > - remove unused imports > - First pass at remove DocCommentTransformer from the public API. > >It is still declared internally, and installed by default, using the > service-provider mechanism. >If the standard impl is not available, an identity transformer is used. > - updates for "first sentence" issues and tests > - add info about provenance of `jdk.internal.md` module > - MarkdownTransformer: tweak doc comments > - MarkdownTransformer: change `Lower.replaceIter` to be `private final` > - MarkdownTransformer: use suggested text for using streams > - ... and 18 more: https://git.openjdk.org/jdk/compare/18e24d06...63dd8bf4 Jon, here are a few reminders and misc comments. - PR Review: https://git.openjdk.org/jdk/pull/16388#pullrequestreview-1846272367
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, 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 [v6]
On Fri, 12 Jan 2024 17:47:06 GMT, Pavel Rappo wrote:
>> Jonathan Gibbons has updated the pull request with a new target base due to
>> a merge or a rebase. The incremental webrev excludes the unrelated changes
>> brought in by the merge/rebase. The pull request contains seven additional
>> commits since the last revision:
>>
>> - 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/api/JavacTrees.java line
> 992:
>
>> 990:
>> 991: private static boolean isMarkdownFile(FileObject fo) {
>> 992: return fo.getName().endsWith(".md");
>
> I wonder why you decided to (re)implement those methods using file extension
> matching. Is it because we don't want to introduce anything Markdown-related
> to this method that was used to implement `isHtmlFile` previously?
>
> https://github.com/openjdk/jdk/blob/8eb4e7e07e9211aabcb0f22696e9c572dac7a59f/src/jdk.compiler/share/classes/com/sun/tools/javac/file/BaseFileManager.java#L489-L498
Musing on this more.
Can/should we, without introducing probably unwelcome `Kind.MD` to
`javax.tools.JavaFileObject.Kind`, teach javac to recognise `package.md`
similarly to how it recognises legacy `package.html`? If we are aiming for
Markdown to be a drop in replacement for traditional javadoc comments, we might
want to go the extra mile.
I'm pleased to see that Markdown `-overview` files work just fine.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1459349927
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v6]
On Mon, 8 Jan 2024 21:26:50 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 incremental webrev excludes the unrelated changes
> brought in by the merge/rebase. The pull request contains seven additional
> commits since the last revision:
>
> - 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 259:
> 257: LineKind lineKind = textKind == DocTree.Kind.MARKDOWN ?
> peekLineKind() : null;
> 258:
> 259: if (DEBUG) System.err.println("starting content " + showPos(bp)
> + " " + newline);
Debug output is useful. I wonder if we should consider
https://openjdk.org/jeps/264.
src/jdk.compiler/share/classes/com/sun/tools/javac/parser/DocCommentParser.java
line 1295:
> 1293: switch (ch) {
> 1294: case ' ' -> indent++;
> 1295: case '\t' -> indent = 4;
Shouldn't it be like this or it does not matter?
```suggestion
case '\t' -> indent += 4;
src/jdk.compiler/share/classes/com/sun/tools/javac/parser/DocCommentParser.java
line 1336:
> 1334: switch (initialLineKind) {
> 1335: case CODE_FENCE -> {
> 1336: if (lineKind == LineKind.CODE_FENCE && ch
> == term && count(ch) == count) {
https://spec.commonmark.org/0.30/#example-124 shows that the closing fence may
be longer than the opening one: consider `count(ch) >= count`.
That said, I note that on my experiment the resulting output was identical with
or without the change I ask you to consider. Perhaps I haven't yet understood
how the parsing works.
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}( .*|$)")),
Nothing wrong here, I just didn't know that an ATX header "opening sequence of
# characters" can be followed by the end of line". Interesting.
src/jdk.compiler/share/classes/com/sun/tools/javac/parser/DocCommentParser.java
line 1421:
> 1419: case '#', '=', '-', '+', '_', '`', '~' -> {
> 1420: String line = peekLine();
> 1421: for (LineKind lk : LineKind.values()) {
Nothing wrong here, just noting that this is one more way one can depend on an
enum constant order. Change it, and a peeked line kind might change too (e.g.
`OTHER` matches everything.)
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1458858629
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1452560905
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1452650328
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1452629053
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1452632699
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v6]
On Mon, 8 Jan 2024 21:26:50 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 incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains seven additional > commits since the last revision: > > - 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 Jon, please sync this PR with mainline to resolve conflicts. - PR Comment: https://git.openjdk.org/jdk/pull/16388#issuecomment-1900181880
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v6]
On Mon, 8 Jan 2024 21:26:50 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 incremental webrev excludes the unrelated changes
> brought in by the merge/rebase. The pull request contains seven additional
> commits since the last revision:
>
> - 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
Here's another batch of comments. Please update `@since 22` to `@since 23`
throughout this PR.
src/jdk.compiler/share/classes/com/sun/source/doctree/DocTreeVisitor.java line
257:
> 255: *
> 256: * @implSpec Visits the provided {@code RawTextTree} node
> 257: * by calling {@code visitOther(node, p)}.
Nit: for consistency with the rest of the file, reorder `@param`-`@return`
block with the `@implSpec` tag:
Suggestion:
*
* @implSpec Visits the provided {@code RawTextTree} node
* by calling {@code visitOther(node, p)}.
*
* @param node the node being visited
* @param p a parameter value
* @return a result value
src/jdk.compiler/share/classes/com/sun/source/doctree/RawTextTree.java line 40:
> 38: * @apiNote
> 39: * This class may be used to represent tree nodes containing
> 40: * {@linkplain DocTree.Kind#MARKDOWN Markdown} text.
This means that there is one-to-many relationship between `RawTextTree` and
`DocTree.KIND`. This in turn perpetuates the pattern of checking the kind
followed by casting as opposed to more modern `instanceof` pattern matching.
There's nothing wrong with it per se, however I wonder what the rationale is
for leaving this part of the API open-ended. Is it to support other types of
raw text in the future?
src/jdk.compiler/share/classes/com/sun/source/util/DocTreeFactory.java line 299:
> 297: * @param code the code
> 298: * @return a {@code RawTextTree} object
> 299: * @throws IllegalArgumentException if the kind is not a recognized
> kind for raw text
This method's implementation also throws `NullPointerException` if kind is
null, which is unusual for these methods. You can either add `@throws`, or
workaround it by using `String.valueOf(kind)` instead of `kind.toString()` down
in the implementation.
src/jdk.compiler/share/classes/com/sun/source/util/DocTreeFactory.java line 303:
> 301: * @since 22
> 302: */
> 303: RawTextTree newRawTextTree(DocTree.Kind kind, String code) throws
> IllegalArgumentException;
It's unusual for a JDK method to declare a runtime exception.
src/jdk.compiler/share/classes/com/sun/source/util/DocTrees.java line 97:
> 95: */
> 96: public enum CommentKind {
> 97: /** The style of comments whose lines are prefixed by{@code ///}.
> */
Suggestion:
/** The style of comments whose lines are prefixed by {@code ///}. */
src/jdk.compiler/share/classes/com/sun/source/util/DocTrees.java line 104:
> 102:
> 103: /**
> 104: * {@return the style of the documentation comment associated with a
> tree node.}
Period is redundant:
Suggestion:
* {@return the style of the documentation comment associated with a tree
node}
src/jdk.compiler/share/classes/com/sun/source/util/DocTrees.java line 111:
> 109: * @since 22
> 110: */
> 111: public abstract CommentKind getDocCommentKind(TreePath path);
This method's specification says nothing about `null` that the implementation
can return.
src/jdk.compiler/share/classes/com/sun/source/util/DocTrees.java line 157:
> 155: * @param fileObject the content container
> 156: * @return the doc comment tree
> 157: * @throws IllegalArgumentException if the file type is not supported
It seems like this exception could've been thrown before, it's just that you
have documented it for the first time. This might be important for CSR.
src/jdk.compiler/share/classes/com/sun/source/util/DocTrees.java line 182:
> 180: * @return the doc comment tree
> 181: * @throws IOException if an exception occurs
> 182: * @throws IllegalArgumentException if the file type is not supported
Ditto on this newly documented exception.
src/jdk.compiler/share/classes/com/sun/source/util/DocTrees.java line 193:
> 191: *
> 192: * Supported file types are HTML files and Markdown
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v6]
On Mon, 8 Jan 2024 21:26:50 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 incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains seven additional > commits since the last revision: > > - 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 On CommonMark. * `jdk.internal.md` contains 133 files, the vast majority of which are from commonmark-java 0.21.0. According to https://github.com/commonmark/commonmark-java/releases 0.21.0 is the latest/current release; good. Questions: * Did we take the tagged commit or mainline at some point after the tagged commit? If it's the latter, we need to take the tagged version. * What's the difference between those commonmark-java files in this PR and official commonmark-java? In other words, how do we adapt them? It would be nice to have a description of the procedure or a script to update those files. * `jdk.internal.md` exports packages to `jdk.jshell`. A question for @lahodaj, who maintains `jdk.jshell`: when do we need to create a new PR similar to that withdrawn https://github.com/openjdk/jdk/pull/11936? src/jdk.internal.md/share/classes/jdk/internal/markdown/MarkdownTransformer.java line 2: > 1: /* > 2: * Copyright (c) 2005, 2023, Oracle and/or its affiliates. All rights > reserved. It's surprising to see 2005. src/jdk.internal.md/share/classes/module-info.java line 29: > 27: * Internal support for Markdown. > 28: * > 29: * @since 22 Suggestion: * @since 23 - PR Review: https://git.openjdk.org/jdk/pull/16388#pullrequestreview-1818084469 PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1450342017 PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1450347103
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v2]
On Wed, 15 Nov 2023 18:48:59 GMT, Jonathan Gibbons wrote: >> Hmm. teeny-tiny "yes", dominated by a big "BUT". >> >> There is no easy simple direct support for Markdown in user-defined taglets, >> since there is no way to know the syntactic form of user-defined tags. We >> might be able to do something for user-defined tags given on the command >> line (with `-tag`) but in general we should be wary and think carefully >> about putting any headings inside any block tag, because block tags get >> converted to HTML definition lists. >> >> --- >> >> Separately, generally speaking, the Markdown headings in any doc comment >> should start at level 1 and increase from there. An offset will be added >> during translation to give the correct heading level in the overall page. >> There is a guard in the code (but no warning as yet) if you "overflow" >> heading level 6. For example, if you go overboard and use ` my heading` >> in the comment for a method (where the offset for headings is 3), it will >> (currently) max out at level 6. Generating warnings for questionable >> Markdown is somewhat against the spirit of Markdown. It would seem a bit >> weird to warn against an obscure case like overflowing headings when the >> general policy for real errors is no message and just show the literal text. > > Update: > Markdown in tags defined by the user on the command-line works pretty much as > expected: not great, but not bad. > > Headings in user-defined tags work, but are semantically questionable, since > the tags are generated inside a definition list (which itself is maybe > questionable, these days.). There is (currently) no special accommodation for > the "virtual heading" in the presentation of the block tag. > > Headings, sections, HTML 5, Markdown and taglets are a complicated mess, and > probably better discussed and tracked in JBS. Understood. FWIW, here are a few examples of headings in user-defined tags in JDK: https://github.com/openjdk/jdk/blob/a6785e4d633908596ddb6de6d2eeab1c9ebdf2c3/src/java.base/share/classes/java/math/BigDecimal.java#L229-L239 https://github.com/openjdk/jdk/blob/ddbbd36e4b064b9e7433f0a55973d72cd6dbc0d3/src/java.xml/share/classes/module-info.java#L402-L420 https://github.com/openjdk/jdk/blob/6f6486e97743eadfb20b4175e1b4b2b05b59a17a/src/jdk.incubator.vector/share/classes/jdk/incubator/vector/Vector.java#L1089-L1093 - PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1449039906
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v6]
On Wed, 8 Nov 2023 16:24:20 GMT, Pavel Rappo wrote: >> Jonathan Gibbons has updated the pull request with a new target base due to >> a merge or a rebase. The incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. The pull request contains seven additional >> commits since the last revision: >> >> - 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 > > test/langtools/tools/javac/classfiles/attributes/deprecated/DeprecatedTest.java > line 26: > >> 24: /* >> 25: * @test >> 26: * @bug 8042261 8298405 > > This comment is not for this line, but for two compiler tests for > `@deprecated` javadoc tag. It seemed quite contextual place to put it. > > Did I miss it, or you are planning to add a javadoc test that verifies that > `@deprecated` appearing in a `///` comment has no [special meaning] it has in > classic `/** */` comments? > > [special meaning]: > https://github.com/openjdk/jdk/blob/128363bf3b57dfa05b3807271b47851733c1afb9/src/jdk.compiler/share/classes/com/sun/tools/javac/parser/JavaTokenizer.java#L1639-L1653 Ping. I do believe that it's important to have such a test. - PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1449001056
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v6]
On Wed, 15 Nov 2023 00:21:10 GMT, Jonathan Gibbons wrote: > the output with the break iterator is now the same as the default simple > iterator I can confirm that. Thanks. - PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1448993923
Re: RFR: 8322936: Update blessed-modifier-order.sh for default, sealed, and non-sealed
On Thu, 11 Jan 2024 12:29:01 GMT, Magnus Ihse Bursie wrote: > @pavelrappo > > > I've run the updated script on JDK. The script found 8 missorted sealed and > > 20 missorted default. The script also found 3 false positive default:[...] > > None of those findings are addressed in this PR. > > Are you planning on addressing this in a separate PR? If not, maybe you can > at least open an issue in JBS. I was not planning to do that, but I sure can. I'll try to publish the PR within a week. Thanks. - PR Comment: https://git.openjdk.org/jdk/pull/17242#issuecomment-1887338396
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v6]
On Wed, 8 Nov 2023 15:57: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 incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. The pull request contains seven additional >> commits since the last revision: >> >> - 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 > > test/langtools/tools/javac/doctree/DocCommentTester.java line 1012: > >> 1010: //} >> 1011: //return null; >> 1012: //} > > Debugging leftover? If you want to leave it for debugging you can make it private and uncomment. > test/langtools/tools/javac/doctree/MarkdownTest.java line 555: > >> 553: // block tags: empty >> 554: //] >> 555: //*/ > > Just to clarify: it is supposed to be commented out, right? If uncommented, > this test fails with a slightly different error. Please update the DocComment printout in that commented out test: the actual content is different. It would be nice if the test were passing at least at the moment of its initial commit. Here's what I see locally: Expect: DocComment[DOC_COMMENT, pos:0 firstSentence: 1 Summary[SUMMARY, pos:4 summary: 1 Erroneous[ERRONEOUS, pos:14, prefPos:37 code: compiler.err.dc.unterminated.inline.tag body: abc_`|_def}|_rest_`more` ] ] body: empty block tags: empty ] Found: DocComment[DOC_COMMENT, pos:0 firstSentence: 1 Summary[SUMMARY, pos:1 summary: 1 Erroneous[ERRONEOUS, pos:11, prefPos:32 code: compiler.err.dc.unterminated.inline.tag body: abc_`|def}|rest_`more` ] ] body: empty block tags: empty ] - PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1447662029 PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1447659363
Integrated: 8322936: Update blessed-modifier-order.sh for default, sealed, and non-sealed
On Wed, 3 Jan 2024 12:09:42 GMT, Pavel Rappo wrote: > Please review this PR to update `blessed-modifier.order.sh` for the > `default`, `sealed`, `non-sealed`, and `strictfp` modifiers. > > In a [discussion][] preceding this PR, it was agreed that the script should > better refer to relevant JLS sections rather than to > [`java.lang.reflect.Modifier#toString`][], which does not model Java source. > > - the `sealed` and `non-sealed` class or interface modifiers were introduced > in JEP 409, but have never been accounted for > > - the `default` method modifier was introduced in Java 8, but has not been > explicitly accounted for. It's unclear whether it was an omission or a > conscious decision. The current edition of JLS [suggests] that in compilable, > modern and customary formatted code, `default` appears alone and, thus, is > always canonically ordered. > > However, a somewhat similar argument applies to the `public` modifier on an > interface method. Its use is discouraged, yet the script would enforce its > order if `public` appears not alone. > > So for completeness, this PR proposes to explicitly account for `default`. > > * While not proposing to do anyting about it, this PR recognises (for the > record) that `strictfp` class, interface, or method modifier became > [obsolete][] in JDK 17. > > I've run the updated script on JDK. The script found 8 missorted `sealed` and > 20 missorted `default`. The script also found 3 false positive `default`: > > - > https://github.com/openjdk/jdk/blob/249d553659ab75a2271e98c77e7d62f662ffa684/src/java.desktop/share/classes/java/awt/dnd/DragSource.java#L526-L527 > - > https://github.com/openjdk/jdk/blob/98da03af50e2372817a7b5e381eea5ee6f2cb919/src/java.management/share/classes/javax/management/MBeanServerFactory.java#L91 > - > https://github.com/openjdk/jdk/blob/6765f902505fbdd02f25b599f942437cd805cad1/src/java.desktop/share/classes/javax/print/PrintServiceLookup.java#L193 > > None of those findings are addressed in this PR. > > [discussion]: > https://mail.openjdk.org/pipermail/core-libs-dev/2024-January/117347.html > [`java.lang.reflect.Modifier#toString`]: > https://docs.oracle.com/en/java/javase/21/docs/api/java.base/java/lang/reflect/Modifier.html#toString(int) > [suggests]: > https://docs.oracle.com/javase/specs/jls/se21/html/jls-9.html#jls-9.4 > [obsolete]: https://openjdk.org/jeps/306 This pull request has now been integrated. Changeset: 37a61720 Author:Pavel Rappo URL: https://git.openjdk.org/jdk/commit/37a61720b60a503a958b35c422ca4f2eb06d62fb Stats: 9 lines in 1 file changed: 6 ins; 0 del; 3 mod 8322936: Update blessed-modifier-order.sh for default, sealed, and non-sealed Reviewed-by: erikj, rriggs, martin - PR: https://git.openjdk.org/jdk/pull/17242
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v5]
On Wed, 15 Nov 2023 18:58:43 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 incrementally with one > additional commit since the last revision: > > Address review comments @jonathan-gibbons, please sync this PR with mainline. As for Skara bots, this comment will hopefully deter them for another 4 weeks. - PR Comment: https://git.openjdk.org/jdk/pull/16388#issuecomment-1881040100
Re: RFR: 8322936: Update blessed-modifier-order.sh for default, sealed, and non-sealed
On Thu, 4 Jan 2024 07:15:32 GMT, Martin Buchholz wrote: > Thanks ... now I see ... FWIW, @jddarcy thinks we can still improve `Modifier.toString(int)`. See this comment and its parent thread: https://github.com/openjdk/jdk/pull/17239#discussion_r1441050656 - PR Review Comment: https://git.openjdk.org/jdk/pull/17242#discussion_r1441537018
Re: RFR: 8322936: Update blessed-modifier-order.sh for default, sealed, and non-sealed
On Wed, 3 Jan 2024 17:16:37 GMT, Martin Buchholz wrote: > it also needs a corresponding update > https://download.java.net/java/early_access/jdk23/docs/api/java.base/java/lang/reflect/Modifier.html#toString(int) >From the preceding discussion, it follows that it cannot be done: >https://mail.openjdk.org/pipermail/core-libs-dev/2024-January/117381.html - PR Review Comment: https://git.openjdk.org/jdk/pull/17242#discussion_r1440721055
RFR: 8322936: Update blessed-modifier-order.sh for default, sealed, and non-sealed
Please review this PR to update `blessed-modifier.order.sh` for the `default`, `sealed`, `non-sealed`, and `strictfp` modifiers. In a [discussion][] preceding this PR, it was agreed that the script should better refer to relevant JLS sections rather than to [`java.lang.reflect.Modifier#toString`][], which does not model Java source. - the `sealed` and `non-sealed` class or interface modifiers were introduced in JEP 409, but have never been accounted for - the `default` method modifier was introduced in Java 8, but has not been explicitly accounted for. It's unclear whether it was an omission or a conscious decision. The current edition of JLS [suggests] that in compilable, modern and customary formatted code, `default` appears alone and, thus, is always canonically ordered. However, a somewhat similar argument applies to the `public` modifier on an interface method. Its use is discouraged, yet the script would enforce its order if `public` appears not alone. So for completeness, this PR proposes to explicitly account for `default`. * While not proposing to do anyting about it, this PR recognises (for the record) that `strictfp` class, interface, or method modifier became [obsolete][] in JDK 17. I've run the updated script on JDK. The script found 8 missorted `sealed` and 20 missorted `default`. The script also found 3 false positive `default`: - https://github.com/openjdk/jdk/blob/249d553659ab75a2271e98c77e7d62f662ffa684/src/java.desktop/share/classes/java/awt/dnd/DragSource.java#L526-L527 - https://github.com/openjdk/jdk/blob/98da03af50e2372817a7b5e381eea5ee6f2cb919/src/java.management/share/classes/javax/management/MBeanServerFactory.java#L91 - https://github.com/openjdk/jdk/blob/6765f902505fbdd02f25b599f942437cd805cad1/src/java.desktop/share/classes/javax/print/PrintServiceLookup.java#L193 None of those findings are addressed in this PR. [discussion]: https://mail.openjdk.org/pipermail/core-libs-dev/2024-January/117347.html [`java.lang.reflect.Modifier#toString`]: https://docs.oracle.com/en/java/javase/21/docs/api/java.base/java/lang/reflect/Modifier.html#toString(int) [suggests]: https://docs.oracle.com/javase/specs/jls/se21/html/jls-9.html#jls-9.4 [obsolete]: https://openjdk.org/jeps/306 - Commit messages: - Initial commit Changes: https://git.openjdk.org/jdk/pull/17242/files Webrev: https://webrevs.openjdk.org/?repo=jdk=17242=00 Issue: https://bugs.openjdk.org/browse/JDK-8322936 Stats: 9 lines in 1 file changed: 6 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/17242.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17242/head:pull/17242 PR: https://git.openjdk.org/jdk/pull/17242
Re: RFR: 8308715: Create a mechanism for Implicitly Declared Class javadoc
On Tue, 28 Nov 2023 14:32:14 GMT, Pavel Rappo wrote:
> Please review this PR to support _JEP 463 Implicitly Declared Classes and
> Instance Main Method (Second Preview)_ in JavaDoc.
>
> [JEP 463](https://openjdk.org/jeps/463) is the next iteration of [JEP
> 445](https://openjdk.org/jeps/445), which introduced the ability to have a
> source file as a launchable, "classless" method bag
>
>
> % cat HelloWorld.java
> /** Run me. */
> void main() {
> print("Hello, world!");
> }
>
> /** Shortcut for printing strings. */
> void print(String arg) {
> System.out.println(arg);
> }
>
>
> which the compiler interprets as a familiar class
>
>
> final class HelloWorld {
>
> HelloWorld() {
> }
>
> /** Run me. */
> void main() {
> print("Hello, world!");
> }
>
> /** Shortcut for printing strings. */
> void print(String arg) {
> System.out.println(arg);
> }
> }
>
>
> ### How JEP 445 works with JavaDoc today
>
> In JDK 21, javadoc can document such a file **without any changes to the
> javadoc tool**. The only thing that the user needs to do is to make sure that
> the following options are present:
>
> * `--enable-preview` and `--source=21`
> * `-package`
>
> The first pair of options tells javadoc to use preview features, which JEP
> 445 is one of. Without these preview-related options, javadoc will raise the
> following error:
>
>
> % javadoc --version
> javadoc 21
>
> % javadoc HelloWorld.java -d /tmp/throwaway
> Loading source file HelloWorld.java...
> HelloWorld.java:2: error: unnamed classes are a preview feature and are
> disabled by default.
> void main() {
> ^
> (use --enable-preview to enable unnamed classes)
> 1 error
>
>
> The second option, `-package`, tells javadoc to document classes that are
> public, protected, or declared with package access (colloquially known as
> "package private"). Without this option, javadoc will only document public
> and protected classes, which do not include the interpreted class:
>
>
> % javadoc --enable-preview --source=21 HelloWorld.java -d /tmp/throwaway
> Loading source file HelloWorld.java...
> Constructing Javadoc information...
> error: No public or protected classes found to document.
> 1 error
>
>
> When those additional options are present, javadoc does its job:
>
>
> % javadoc --enable-preview --source=21 -package HelloWorld.java -d
> /tmp/throwaway
> Loading source file HelloWorld.java...
> Constructing Javadoc information...
> Creating destination directory: "/tmp/throwaway/"
> Building index for all the packages and classes...
> Standard Doclet version 21+35-LTS-2513
> Building tree for all the packages and classes...
> Generating /tmp/throwaway/HelloWorld.htm...
Reviewers, please ignore changes to the following files as well as commits that
brought them:
* .github/workflows/main.yml
* src/hotspot/share/utilities/nativeCallStack.cpp
Those changes seem to be transient artefacts of the workflow and should
disappear on their own, eventually.
The reason those changes appeared in this PR is that this PR's branch is based
on a more recent master than that of the PR that this PR depends on,
https://github.com/openjdk/jdk/pull/16461.
-
PR Comment: https://git.openjdk.org/jdk/pull/16853#issuecomment-1829986363
RFR: 8308715: Create a mechanism for Implicitly Declared Class javadoc
Please review this PR to support _JEP 463 Implicitly Declared Classes and
Instance Main Method (Second Preview)_ in JavaDoc.
[JEP 463](https://openjdk.org/jeps/463) is the next iteration of [JEP
445](https://openjdk.org/jeps/445), which introduced the ability to have a
source file as a launchable, "classless" method bag
% cat HelloWorld.java
/** Run me. */
void main() {
print("Hello, world!");
}
/** Shortcut for printing strings. */
void print(String arg) {
System.out.println(arg);
}
which the compiler interprets as a familiar class
final class HelloWorld {
HelloWorld() {
}
/** Run me. */
void main() {
print("Hello, world!");
}
/** Shortcut for printing strings. */
void print(String arg) {
System.out.println(arg);
}
}
### How JEP 445 works with JavaDoc today
In JDK 21, javadoc can document such a file **without any changes to the
javadoc tool**. The only thing that the user needs to do is to make sure that
the following options are present:
* `--enable-preview` and `--source=21`
* `-package`
The first pair of options tells javadoc to use preview features, which JEP 445
is one of. Without these preview-related options, javadoc will raise the
following error:
% javadoc --version
javadoc 21
% javadoc HelloWorld.java -d /tmp/throwaway
Loading source file HelloWorld.java...
HelloWorld.java:2: error: unnamed classes are a preview feature and are
disabled by default.
void main() {
^
(use --enable-preview to enable unnamed classes)
1 error
The second option, `-package`, tells javadoc to document classes that are
public, protected, or declared with package access (colloquially known as
"package private"). Without this option, javadoc will only document public and
protected classes, which do not include the interpreted class:
% javadoc --enable-preview --source=21 HelloWorld.java -d /tmp/throwaway
Loading source file HelloWorld.java...
Constructing Javadoc information...
error: No public or protected classes found to document.
1 error
When those additional options are present, javadoc does its job:
% javadoc --enable-preview --source=21 -package HelloWorld.java -d
/tmp/throwaway
Loading source file HelloWorld.java...
Constructing Javadoc information...
Creating destination directory: "/tmp/throwaway/"
Building index for all the packages and classes...
Standard Doclet version 21+35-LTS-2513
Building tree for all the packages and classes...
Generating /tmp/throwaway/HelloWorld.html...
HelloWorld.java:7: warning: no @param for arg
void print(String arg) {
^
HelloWorld.java:2: warning: no comment
void main() {
^
HelloWorld.java:2: warning: use of default constructor, which does not provide
a comment
void main() {
^
Generating /tmp/throwaway/package-summary.html...
Generating /tmp/throwaway/package-tree.html...
Generating /tmp/throwaway/overview-tree.html...
Building index for all classes...
Generating /tmp/throwaway/allclasses-index.html...
Generating /tmp/throwaway/allpackages-index.html...
Generating /tmp/throwaway/index-all.html...
Generating /tmp/throwaway/search.html...
Generating /tmp/throwaway/index.html...
Generating /tmp/throwaway/help-doc.html...
3 warnings
However, the result does not feel quite right. Firstly, `-package` is too
coarse. It includes all top-level classes and their elements, not just the
implicit class from `HelloWorld.java`, its default constructor and methods,
which are all declared with package access. Secondly, `HelloWorld.java` isn't a
first-class citizen in javadoc. That latter fact can be seen from examining
stdout and the output directory:
1. DocLint (compiler and javadoc) as well as javadoc itself issue unjust
warnings: neither the implicit class nor its default constructor can be
documented. The author either does not know about classes and constructors yet
(on-ramp audience) or does not care about them (scripts/utilities audience).
Additionally, because the class' AST node is at the same position as that of
the first method declaration, the warning about the undocumented class can be
confused with a warning on the first method being undocumented.
2. While such a file is documented as if it were an explicitly declared
(normal) class, we might want to dispense with the documentation for the
default constructor as it lacks a comment and is an artefact.
### What this PR proposes for JEP 463
1. Leave `--enable-preview` and `--source` as correct and unavoidable until the
feature is standardised.
2. "Drill a hole" in javadoc access control to **automatically** allow implicit
classes and their public, protected or declared with package access members in
documentation.
3. Do not emit warnings for an implicit class and its deault constructor.
4. Do not document an implicit class' default constructor.
-
Depends on: https://git.openjdk.org/jdk/pull/16461
Commit messages:
- Initial commit
- 8320358: GHA: ignore jdk* branches
- 8319437:
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v2]
On Wed, 8 Nov 2023 22:17:21 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 incrementally with one > additional commit since the last revision: > > Customize support for Markdown headings I see you've added a new feature and tests for it. test/langtools/jdk/javadoc/doclet/testMarkdown/TestMarkdown.java line 613: > 611: /// Lorem ipsum. > 612: /// > 613: /// ## ATX-style subheading for executable Can we use Markdown headings inside block tags, such as `@apiNote`? If so, should they start from h5? If so, should we get a warning if they "overflow" h6? - PR Review: https://git.openjdk.org/jdk/pull/16388#pullrequestreview-1723026055 PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1388270695
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments
On Wed, 8 Nov 2023 17:25:36 GMT, Pavel Rappo 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
>
> test/langtools/jdk/javadoc/tool/testTransformer/TestTransformer.java line 64:
>
>> 62:
>> 63: @Test
>> 64: public void testFindStandardTransformer_raw() throws Exception {
>
> Checked exceptions are not thrown:
> Suggestion:
>
> public void testFindStandardTransformer_raw() {
I might be mistaken, but this and the testFindStandardTransformer_stream
methods look like we are testing ServiceLoader API. I would leave just the
stream version.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1386974146
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments
On Thu, 26 Oct 2023 23:29:00 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
I'll review this PR's tests first. Most of them look good. For the rest,
comments inline.
test/langtools/jdk/javadoc/tool/sampleapi/lib/sampleapi/generator/ModuleGenerator.java
line 49:
> 47: import sampleapi.util.PoorDocCommentTable;
> 48:
> 49: import static
> com.sun.tools.javac.parser.Tokens.Comment.CommentStyle.JAVADOC;
To clarify, the change to this file is that you removed two unused imports,
right?
test/langtools/jdk/javadoc/tool/testTransformer/TestTransformer.java line 37:
> 35:
> 36: import java.nio.file.Path;
> 37: import java.util.ArrayList;
Import in unused:
Suggestion:
test/langtools/jdk/javadoc/tool/testTransformer/TestTransformer.java line 61:
> 59: }
> 60:
> 61: ToolBox tb = new ToolBox();
Suggestion:
private final ToolBox tb = new ToolBox();
test/langtools/jdk/javadoc/tool/testTransformer/TestTransformer.java line 64:
> 62:
> 63: @Test
> 64: public void testFindStandardTransformer_raw() throws Exception {
Checked exceptions are not thrown:
Suggestion:
public void testFindStandardTransformer_raw() {
test/langtools/jdk/javadoc/tool/testTransformer/TestTransformer.java line 82:
> 80:
> 81: @Test
> 82: public void testFindStandardTransformer_stream() throws Exception {
Checked exceptions are not thrown here too:
Suggestion:
public void testFindStandardTransformer_stream() {
test/langtools/jdk/javadoc/tool/testTransformer/TestTransformer.java line 96:
> 94: var sl =
> ServiceLoader.load(DocTrees.DocCommentTreeTransformer.class);
> 95: return StreamSupport.stream(sl.spliterator(), false)
> 96: .filter(p -> p.name().equals(name))
ServiceLoader specification suggests another way of streaming:
Suggestion:
return sl.stream()
.map(ServiceLoader.Provider::get)
.filter(t -> t.name().equals(name))
Would it be the same and more readable?
test/langtools/tools/javac/classfiles/attributes/deprecated/DeprecatedTest.java
line 26:
> 24: /*
> 25: * @test
> 26: * @bug 8042261 8298405
This comment is not for this line, but for two compiler tests for `@deprecated`
javadoc tag. It seemed quite contextual place to put it.
Did I miss it, or you are planning to add a javadoc test that verifies that
`@deprecated` appearing in a `///` comment has no [special meaning]() it has in
classic `/** */` comments?
[special meaning]:
https://github.com/openjdk/jdk/blob/128363bf3b57dfa05b3807271b47851733c1afb9/src/jdk.compiler/share/classes/com/sun/tools/javac/parser/JavaTokenizer.java#L1639-L1653
test/langtools/tools/javac/doctree/DocCommentTester.java line 1012:
> 1010: //}
> 1011: //return null;
> 1012: //}
Debugging leftover?
test/langtools/tools/javac/doctree/FirstSentenceTest.java line 423:
> 421: DocComment[DOC_COMMENT, pos:0
> 422: firstSentence: 1
> 423: RawText[MARKDOWN, pos:0, abc.|def.]
It took me a while to understand why this test expects the first sentence to
include the second line:
///abc.
///def.
void simpleMarkdown() { }
It's not because it's Markdown or the new `///` comment syntax. It's because
the breakiterator thinks that `abc.\ndef.` is one sentence.
Now, in this same file, on [line
123](https://github.com/openjdk/jdk/blob/128363bf3b57dfa05b3807271b47851733c1afb9/test/langtools/tools/javac/doctree/FirstSentenceTest.java#L119-L142),
there's this test:
/**
* abc def ghi.
* jkl mno pqr
*/
void dot_newline() { }
If you compare its expectation to that of simpleMarkdown(), you'll see that the
first sentence consists of the first line only:
BREAK_ITERATOR
DocComment[DOC_COMMENT, pos:1
firstSentence: 1
Text[TEXT, pos:1, abc_def_ghi.]
body: 1
Text[TEXT, pos:15, jkl_mno_pqr]
block tags: empty
]
So, why does it seem to work differently for `///` and `/** */` comments? It
turns out that a whitespace character immediately after newline is important
for breakiterator to do its thing:
```
abc def ghi.\n jkl mno pqr
^
The problem with the simpleMarkdown test is that we cannot just indent the
comment. That indentation will be deemed incidental whitespace and, thus, will
removed. For example, suppose we do this:
/// abc.
/// def.
void simpleMarkdown() { }
The breakiterator will still see `abc.\ndef.`. Of course, we could do this:
///abc.
/// def.
void simpleMarkdown() { }
But it
Integrated: 8314753: Remove support for @beaninfo, @ToDo, @since.unbundled, and @Note
On Tue, 22 Aug 2023 11:09:39 GMT, Pavel Rappo wrote: > Please review this trivial PR. This pull request has now been integrated. Changeset: a4e97aa4 Author: Pavel Rappo URL: https://git.openjdk.org/jdk/commit/a4e97aa4ebe6fcfc3ed9e45ed81df1d55e52d621 Stats: 4 lines in 1 file changed: 0 ins; 4 del; 0 mod 8314753: Remove support for @beaninfo, @ToDo, @since.unbundled, and @Note Reviewed-by: rriggs, azvegint, kevinw - PR: https://git.openjdk.org/jdk/pull/15385
Integrated: 8314762: Make {@Incubating} conventional
On Tue, 22 Aug 2023 12:19:42 GMT, Pavel Rappo wrote:
> Please review this trivial PR.
This pull request has now been integrated.
Changeset: 0901d75e
Author: Pavel Rappo
URL:
https://git.openjdk.org/jdk/commit/0901d75e074322c5a8d55e3c72c4cba4291fb00c
Stats: 6 lines in 3 files changed: 0 ins; 0 del; 6 mod
8314762: Make {@Incubating} conventional
Reviewed-by: jjg, iris, chegar
-
PR: https://git.openjdk.org/jdk/pull/15387
Re: RFR: 8314753: Remove support for @beaninfo, @ToDo, @since.unbundled, and @Note [v2]
On Tue, 22 Aug 2023 14:55:18 GMT, Pavel Rappo wrote: >> Please review this trivial PR. > > Pavel Rappo has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains two commits: > > - Merge branch 'master' into 8314753 > - Initial commit Thanks for reviewing it. > Looks trivial only after reviewing the issue and knowing the background. > The PR description could be a bit more complete and save a bunch of clicking > around. Fair enough. For the benefit of other reviewers, I'll copy the JBS description here and additionally note that tags in question are absent in the mainline doc comments and are also disabled during `make docs`. JBS: Those tags seem to have been effectively decommissioned, but their remnants are still there and when seen, raise needless questions. - `@beaninfo` seems to relate to UI: - [JDK-7179078](https://bugs.openjdk.org/browse/JDK-7179078) - [JDK-4763438](https://bugs.openjdk.org/browse/JDK-4763438) - [JDK-8051548](https://bugs.openjdk.org/browse/JDK-8051548) - `@ToDo` and `@since.unbundled` hasn't been used since the initial load (2007). - `@Note` seems to relate to UI: - [JDK-8285686](https://bugs.openjdk.org/browse/JDK-8285686) - [JDK-8227324](https://bugs.openjdk.org/browse/JDK-8227324) - [JDK-8222362](https://bugs.openjdk.org/browse/JDK-8222362) - PR Comment: https://git.openjdk.org/jdk/pull/15385#issuecomment-1688367797
Re: RFR: 8314753: Remove support for @beaninfo, @ToDo, @since.unbundled, and @Note [v2]
> Please review this trivial PR. Pavel Rappo has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains two commits: - Merge branch 'master' into 8314753 - Initial commit - Changes: https://git.openjdk.org/jdk/pull/15385/files Webrev: https://webrevs.openjdk.org/?repo=jdk=15385=01 Stats: 4 lines in 1 file changed: 0 ins; 4 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/15385.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15385/head:pull/15385 PR: https://git.openjdk.org/jdk/pull/15385
Integrated: 8314738: Remove all occurrences of and support for @revised
On Tue, 22 Aug 2023 08:42:32 GMT, Pavel Rappo wrote: > Please review this simple PR. This pull request has now been integrated. Changeset: f39fc0aa Author: Pavel Rappo URL: https://git.openjdk.org/jdk/commit/f39fc0aa2de19332fa51af605ece0660891d8c7a Stats: 124 lines in 28 files changed: 0 ins; 116 del; 8 mod 8314738: Remove all occurrences of and support for @revised Reviewed-by: mr - PR: https://git.openjdk.org/jdk/pull/15382
Re: RFR: 8314738: Remove all occurrences of and support for @revised
On Tue, 22 Aug 2023 12:23:18 GMT, Mark Reinhold wrote: > I wouldn’t update the copyright year as you have in some of these files. I was taught to do it every time when I change a file. Would you like me to revert those changes to copyright years in this case? - PR Comment: https://git.openjdk.org/jdk/pull/15382#issuecomment-1688086627
RFR: 8314762: Make {@Incubating} conventional
Please review this trivial PR. - Commit messages: - Initial commit Changes: https://git.openjdk.org/jdk/pull/15387/files Webrev: https://webrevs.openjdk.org/?repo=jdk=15387=00 Issue: https://bugs.openjdk.org/browse/JDK-8314762 Stats: 6 lines in 3 files changed: 0 ins; 0 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/15387.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15387/head:pull/15387 PR: https://git.openjdk.org/jdk/pull/15387
Re: RFR: 8314753: Remove support for @beaninfo, @ToDo, @since.unbundled, and @Note
On Tue, 22 Aug 2023 11:09:39 GMT, Pavel Rappo wrote: > Please review this trivial PR. CC'ing client-libs-dev because @beaninfo and @Note and jmx-dev because of @since.unbundled, which might've been used for JMX before 2007. - PR Comment: https://git.openjdk.org/jdk/pull/15385#issuecomment-1688004264
Re: RFR: 8314753: Remove support for @beaninfo, @ToDo, @since.unbundled, and @Note
On Tue, 22 Aug 2023 11:09:39 GMT, Pavel Rappo wrote: > Please review this trivial PR. CC'ing core-libs-dev whose members might also have some recollection on tags in question. - PR Comment: https://git.openjdk.org/jdk/pull/15385#issuecomment-1687995105
Re: RFR: 8314753: Remove support for @beaninfo, @ToDo, @since.unbundled, and @Note
On Tue, 22 Aug 2023 11:09:39 GMT, Pavel Rappo wrote: > Please review this trivial PR. CC'ing client-libs-dev because `@beaninfo` and `@Note` and jmx-dev because of `@since.unbundled`, which might've been used for JMX before 2007. - PR Comment: https://git.openjdk.org/jdk/pull/15385#issuecomment-1687990983
RFR: 8314753: Remove support for @beaninfo, @ToDo, @since.unbundled, and @Note
Please review this trivial PR. - Commit messages: - Initial commit Changes: https://git.openjdk.org/jdk/pull/15385/files Webrev: https://webrevs.openjdk.org/?repo=jdk=15385=00 Issue: https://bugs.openjdk.org/browse/JDK-8314753 Stats: 4 lines in 1 file changed: 0 ins; 4 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/15385.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15385/head:pull/15385 PR: https://git.openjdk.org/jdk/pull/15385
RFR: 8314738: Remove all occurrences of and support for @revised
Please review this simple PR. - Commit messages: - Initial commit Changes: https://git.openjdk.org/jdk/pull/15382/files Webrev: https://webrevs.openjdk.org/?repo=jdk=15382=00 Issue: https://bugs.openjdk.org/browse/JDK-8314738 Stats: 124 lines in 28 files changed: 0 ins; 116 del; 8 mod Patch: https://git.openjdk.org/jdk/pull/15382.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15382/head:pull/15382 PR: https://git.openjdk.org/jdk/pull/15382
Integrated: 8310890: Normalize identifier names
On Mon, 26 Jun 2023 14:07:03 GMT, Pavel Rappo wrote: > Please review this cleanup PR to normalize names of identifiers which are > Java variables/fields or tokens in text files. Those names either contain a > pronoun that is very rarely used in code, or seem like they contain such a > pronoun, which, in fact, they don't. Either way, the goal is to improve > readability and clarity. > > Also, this PR fixes a few related typos. This pull request has now been integrated. Changeset: f6133edb Author:Pavel Rappo URL: https://git.openjdk.org/jdk/commit/f6133edb08dd7a7d764638c5b1cdd5c3e56ed64e Stats: 184 lines in 10 files changed: 0 ins; 0 del; 184 mod 8310890: Normalize identifier names Reviewed-by: naoto, rriggs - PR: https://git.openjdk.org/jdk/pull/14653
Re: RFR: 8310890: Normalize identifier names
On Mon, 26 Jun 2023 18:44:42 GMT, Pavel Rappo wrote:
>> make/data/charsetmapping/charsets line 149:
>>
>>> 147: package sun.nio.cs
>>> 148: typesbcs
>>> 149: histname ISO8859_2
>>
>> Should this column be re-aligned with the longer name?
>
> I thought about it before publishing the PR. I decided not to re-align
> anything because (i) the change would be bigger and (ii) the fact that there
> was already a property that is similarly misaligned; search for:
>
> internal true
If you are concerned with functionality rather than looks, then I can tell you
this:
1. The build succeeds and tier1 tests pass.
2. The code that parses that file expect one or more whitespace characters as a
separator:
String[] tokens = line.split("\\s+");
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14653#discussion_r1242629052
Re: RFR: 8310890: Normalize identifier names
On Mon, 26 Jun 2023 18:31:06 GMT, Jens Lidestrom wrote: >> Please review this cleanup PR to normalize names of identifiers which are >> Java variables/fields or tokens in text files. Those names either contain a >> pronoun that is very rarely used in code, or seem like they contain such a >> pronoun, which, in fact, they don't. Either way, the goal is to improve >> readability and clarity. >> >> Also, this PR fixes a few related typos. > > make/data/charsetmapping/charsets line 149: > >> 147: package sun.nio.cs >> 148: typesbcs >> 149: histname ISO8859_2 > > Should this column be re-aligned with the longer name? I thought about it before publishing the PR. I decided not to re-align anything because (i) the change would be bigger and (ii) the fact that there was already a property that is similarly misaligned; search for: internal true - PR Review Comment: https://git.openjdk.org/jdk/pull/14653#discussion_r1242617579
Re: RFR: 8310890: Normalize identifier names
On Mon, 26 Jun 2023 18:21:07 GMT, Roger Riggs wrote: >> Please review this cleanup PR to normalize names of identifiers which are >> Java variables/fields or tokens in text files. Those names either contain a >> pronoun that is very rarely used in code, or seem like they contain such a >> pronoun, which, in fact, they don't. Either way, the goal is to improve >> readability and clarity. >> >> Also, this PR fixes a few related typos. > > src/java.base/share/classes/java/util/EnumMap.java line 690: > >> 688: Object otherValue = em.vals[i]; >> 689: if (otherValue != ourValue && >> 690: (otherValue == null || !otherValue.equals(ourValue))) > > Is this the same as java.util.Objects: > `!Objects.equals(vals[i], em.vals[i]);` You are right: we can fold this if-else construct into that utility method. That said, it in *this* PR, I'd prefer not to. The reason is that I've been preparing a much bigger PR that uses Objects.equals as well as other utility methods and modern language features to improve some equals, hashCode, and compareTo implementations across java.base. I'm planning to publish that PR soon. In fact, this change to Java variable names was cherry-picked from that bigger PR, to separate trivial renames from code changes. - PR Review Comment: https://git.openjdk.org/jdk/pull/14653#discussion_r1242608041
RFR: 8310890: Normalize identifier names
Please review this cleanup PR to normalize names of identifiers which are Java variables/fields or tokens in text files. Those names either contain a pronoun that is very rarely used in code, or seem like they contain such a pronoun, which, in fact, they don't. Either way, the goal is to improve readability and clarity. Also, this PR fixes a few related typos. - Commit messages: - Improve further - Initial commit Changes: https://git.openjdk.org/jdk/pull/14653/files Webrev: https://webrevs.openjdk.org/?repo=jdk=14653=00 Issue: https://bugs.openjdk.org/browse/JDK-8310890 Stats: 184 lines in 10 files changed: 0 ins; 0 del; 184 mod Patch: https://git.openjdk.org/jdk/pull/14653.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14653/head:pull/14653 PR: https://git.openjdk.org/jdk/pull/14653
Re: Inconsistent revision control
It appears that the error is coming from make/SourceRevision.gmk > On 26 Jun 2023, at 13:55, Alan Snyder wrote: > > It would be very unlikely for CCC to fail to correctly clone the directory. > > It would be helpful to know what the build tool is complaining about with > that message. > >> On Jun 25, 2023, at 10:04 PM, David Holmes wrote: >> >> On 24/06/2023 12:28 pm, Alan Snyder wrote: >>> I have been trying to use Carbon Copy Cloner to make a copy of an active >>> jdk repo on another macOS machine for the purpose of running tests. >>> The initial clone is fine. >>> After an incremental clone of the directory tree, I get mysterious messages >>> like this: >>> Inconsistent revision control: 17-24-55/ is missing .git directory >>> What is this about? >>> Is there an easy way to fix this problem? >> >> This isn't really a build issue. I can only suggest that you check that the >> copy you made actually copied across all hidden directories, e.g. .git, as >> well. >> >> Cheers, >> David >> >
Integrated: 8303480: Miscellaneous fixes to mostly invisible doc comments
On Thu, 2 Mar 2023 12:03:44 GMT, Pavel Rappo wrote: > Please review this superficial documentation cleanup that was triggered by > unrelated analysis of doc comments in JDK API. > > The only effect that this multi-area PR has on the JDK API Documentation > (i.e. the observable effect on the generated HTML pages) can be summarized as > follows: > > > diff -ur build/macosx-aarch64/images/docs-before/api/serialized-form.html > build/macosx-aarch64/images/docs-after/api/serialized-form.html > --- build/macosx-aarch64/images/docs-before/api/serialized-form.html > 2023-03-02 11:47:44 > +++ build/macosx-aarch64/images/docs-after/api/serialized-form.html > 2023-03-02 11:48:45 > @@ -17084,7 +17084,7 @@ > throws href="java.base/java/io/IOException.html" title="class in > java.io">IOException, > ClassNotFoundException > readObject is called to restore the > state of the > - (@code BasicPermission} from a stream. > + BasicPermission from a stream. > > Parameters: > s - the ObjectInputStream from which data > is read > > Notes > - > > * I'm not an expert in any of the affected areas, except for jdk.javadoc, and > I was merely after misused tags. Because of that, I would appreciate reviews > from experts in other areas. > * I discovered many more issues than I included in this PR. The excluded > issues seem to occur in infrequently updated third-party code (e.g. > javax.xml), which I assume we shouldn't touch unless necessary. > * I will update copyright years after (and if) the fix had been approved, as > required. This pull request has now been integrated. Changeset: 45a616a8 Author:Pavel Rappo URL: https://git.openjdk.org/jdk/commit/45a616a891e4a4b0e77b1f2fa040522f4a99d172 Stats: 75 lines in 39 files changed: 0 ins; 0 del; 75 mod 8303480: Miscellaneous fixes to mostly invisible doc comments Reviewed-by: mullan, prr, cjplummer, aivanov, jjg, lancea, rriggs, ihse - PR: https://git.openjdk.org/jdk/pull/12826
Re: RFR: 8303480: Miscellaneous fixes to mostly invisible doc comments [v2]
> Please review this superficial documentation cleanup that was triggered by > unrelated analysis of doc comments in JDK API. > > The only effect that this multi-area PR has on the JDK API Documentation > (i.e. the observable effect on the generated HTML pages) can be summarized as > follows: > > > diff -ur build/macosx-aarch64/images/docs-before/api/serialized-form.html > build/macosx-aarch64/images/docs-after/api/serialized-form.html > --- build/macosx-aarch64/images/docs-before/api/serialized-form.html > 2023-03-02 11:47:44 > +++ build/macosx-aarch64/images/docs-after/api/serialized-form.html > 2023-03-02 11:48:45 > @@ -17084,7 +17084,7 @@ > throws href="java.base/java/io/IOException.html" title="class in > java.io">IOException, > ClassNotFoundException > readObject is called to restore the > state of the > - (@code BasicPermission} from a stream. > + BasicPermission from a stream. > > Parameters: > s - the ObjectInputStream from which data > is read > > Notes > - > > * I'm not an expert in any of the affected areas, except for jdk.javadoc, and > I was merely after misused tags. Because of that, I would appreciate reviews > from experts in other areas. > * I discovered many more issues than I included in this PR. The excluded > issues seem to occur in infrequently updated third-party code (e.g. > javax.xml), which I assume we shouldn't touch unless necessary. > * I will update copyright years after (and if) the fix had been approved, as > required. Pavel Rappo has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains two additional commits since the last revision: - Merge branch 'master' into 8303480 - Initial commit - Changes: - all: https://git.openjdk.org/jdk/pull/12826/files - new: https://git.openjdk.org/jdk/pull/12826/files/d2f4a553..87166408 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=12826=01 - incr: https://webrevs.openjdk.org/?repo=jdk=12826=00-01 Stats: 13433 lines in 415 files changed: 9003 ins; 2610 del; 1820 mod Patch: https://git.openjdk.org/jdk/pull/12826.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12826/head:pull/12826 PR: https://git.openjdk.org/jdk/pull/12826
Re: RFR: 8303480: Miscellaneous fixes to mostly invisible doc comments
On Fri, 3 Mar 2023 08:15:49 GMT, Alexey Ivanov wrote:
>> Please review this superficial documentation cleanup that was triggered by
>> unrelated analysis of doc comments in JDK API.
>>
>> The only effect that this multi-area PR has on the JDK API Documentation
>> (i.e. the observable effect on the generated HTML pages) can be summarized
>> as follows:
>>
>>
>> diff -ur
>> build/macosx-aarch64/images/docs-before/api/serialized-form.html
>> build/macosx-aarch64/images/docs-after/api/serialized-form.html
>> --- build/macosx-aarch64/images/docs-before/api/serialized-form.html
>> 2023-03-02 11:47:44
>> +++ build/macosx-aarch64/images/docs-after/api/serialized-form.html
>> 2023-03-02 11:48:45
>> @@ -17084,7 +17084,7 @@
>> throws > href="java.base/java/io/IOException.html" title="class in
>> java.io">IOException,
>> ClassNotFoundException
>> readObject is called to restore the
>> state of the
>> - (@code BasicPermission} from a stream.
>> + BasicPermission from a stream.
>>
>> Parameters:
>> s - the ObjectInputStream from which data
>> is read
>>
>> Notes
>> -
>>
>> * I'm not an expert in any of the affected areas, except for jdk.javadoc,
>> and I was merely after misused tags. Because of that, I would appreciate
>> reviews from experts in other areas.
>> * I discovered many more issues than I included in this PR. The excluded
>> issues seem to occur in infrequently updated third-party code (e.g.
>> javax.xml), which I assume we shouldn't touch unless necessary.
>> * I will update copyright years after (and if) the fix had been approved, as
>> required.
>
> src/jdk.compiler/share/classes/com/sun/tools/javac/code/Types.java line 2866:
>
>> 2864: * Merge multiple abstract methods. The preferred method is a
>> method that is a subsignature
>> 2865: * of all the other signatures and whose return type is more
>> specific {@link MostSpecificReturnCheck}.
>> 2866: * The resulting preferred method has a thrown clause that is the
>> intersection of the merged
>
> Is it “…has a {@code throws} clause…”?
Thanks! I'll add this to a separate PR.
-
PR: https://git.openjdk.org/jdk/pull/12826
Re: RFR: 8303480: Miscellaneous fixes to mostly invisible doc comments
On Thu, 2 Mar 2023 16:23:17 GMT, Alexey Ivanov wrote:
>> Please review this superficial documentation cleanup that was triggered by
>> unrelated analysis of doc comments in JDK API.
>>
>> The only effect that this multi-area PR has on the JDK API Documentation
>> (i.e. the observable effect on the generated HTML pages) can be summarized
>> as follows:
>>
>>
>> diff -ur
>> build/macosx-aarch64/images/docs-before/api/serialized-form.html
>> build/macosx-aarch64/images/docs-after/api/serialized-form.html
>> --- build/macosx-aarch64/images/docs-before/api/serialized-form.html
>> 2023-03-02 11:47:44
>> +++ build/macosx-aarch64/images/docs-after/api/serialized-form.html
>> 2023-03-02 11:48:45
>> @@ -17084,7 +17084,7 @@
>> throws > href="java.base/java/io/IOException.html" title="class in
>> java.io">IOException,
>> ClassNotFoundException
>> readObject is called to restore the
>> state of the
>> - (@code BasicPermission} from a stream.
>> + BasicPermission from a stream.
>>
>> Parameters:
>> s - the ObjectInputStream from which data
>> is read
>>
>> Notes
>> -
>>
>> * I'm not an expert in any of the affected areas, except for jdk.javadoc,
>> and I was merely after misused tags. Because of that, I would appreciate
>> reviews from experts in other areas.
>> * I discovered many more issues than I included in this PR. The excluded
>> issues seem to occur in infrequently updated third-party code (e.g.
>> javax.xml), which I assume we shouldn't touch unless necessary.
>> * I will update copyright years after (and if) the fix had been approved, as
>> required.
>
> src/java.base/share/classes/java/lang/invoke/BootstrapMethodInvoker.java line
> 257:
>
>> 255:
>> 256: /**
>> 257: * @return true iff the BSM method type exactly matches
>
> I assume “iff” should “if”?
Here and elsewhere in this file "iff" might mean [if and only
if](https://en.wikipedia.org/wiki/If_and_only_if), which would make sense.
(FWIW, there are a few hundred occurrences of the word "iff" in src.)
@cl4es (Claes Redestad), as the author of those lines would you like to chime
in?
Since Claes might read this, I note that when I changed unsupported `{@see}` to
`{@link}` thoughtout this file, my IDE could not resolve one of the links:
`java.lang.invoke.LambdaMetafactory#metafactory(MethodHandles.Lookup,String,Class,MethodType,MethodHandle,MethodType)`
While there's a similarly-name method with slightly different parameters, I
refrained from using it:
`java.lang.invoke.LambdaMetafactory#metafactory(MethodHandles.Lookup,String,MethodType,MethodType,MethodHandle,MethodType)`.
-
PR: https://git.openjdk.org/jdk/pull/12826
RFR: 8303480: Miscellaneous fixes to mostly invisible doc comments
Please review this superficial documentation cleanup that was triggered by unrelated analysis of doc comments in JDK API. The only effect that this multi-area PR has on the JDK API Documentation (i.e. the observable effect on the generated HTML pages) can be summarized as follows: diff -ur build/macosx-aarch64/images/docs-before/api/serialized-form.html build/macosx-aarch64/images/docs-after/api/serialized-form.html --- build/macosx-aarch64/images/docs-before/api/serialized-form.html 2023-03-02 11:47:44 +++ build/macosx-aarch64/images/docs-after/api/serialized-form.html 2023-03-02 11:48:45 @@ -17084,7 +17084,7 @@ throws IOException, ClassNotFoundException readObject is called to restore the state of the - (@code BasicPermission} from a stream. + BasicPermission from a stream. Parameters: s - the ObjectInputStream from which data is read Notes - * I'm not an expert in any of the affected areas, except for jdk.javadoc, and I was merely after misused tags. Because of that, I would appreciate reviews from experts in other areas. * I discovered many more issues than I included in this PR. The excluded issues seem to occur in infrequently updated third-party code (e.g. javax.xml), which I assume we shouldn't touch unless necessary. * I will update copyright years after (and if) the fix had been approved, as required. - Commit messages: - Initial commit Changes: https://git.openjdk.org/jdk/pull/12826/files Webrev: https://webrevs.openjdk.org/?repo=jdk=12826=00 Issue: https://bugs.openjdk.org/browse/JDK-8303480 Stats: 75 lines in 39 files changed: 0 ins; 0 del; 75 mod Patch: https://git.openjdk.org/jdk/pull/12826.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12826/head:pull/12826 PR: https://git.openjdk.org/jdk/pull/12826
Re: RFR: 8298050: Add links to graph output for javadoc
On Thu, 8 Dec 2022 16:22:30 GMT, Jonathan Gibbons wrote:
>> make/jdk/src/classes/build/tools/taglet/SealedGraph.java line 207:
>>
>>> 205: private static String relativeLink(TypeElement rootNode,
>>> TypeElement node) {
>>> 206: var backNavigator =
>>> rootNode.getQualifiedName().toString().chars()
>>> 207: .filter(c -> '.' == c)
>>
>> "Yoda conditions" are usually frowned upon. Can `c` be `null`?
>
> `c` is a character
`c` is an `int`, but even that does not matter. I read off `==` but somehow
pictured `equals` in my head; so, null-ness does not matter here at all. That
said, unnecessary "Yoda conditions" should probably be avoided.
-
PR: https://git.openjdk.org/jdk/pull/11580
Re: RFR: 8298050: Add links to graph output for javadoc
On Thu, 8 Dec 2022 09:19:54 GMT, Per Minborg wrote:
> This PR proposes adding hyperlinks to the sealed graphic layout making
> navigation much simpler via the image.
Given that this feature is welcome, I would suggest changing the priority of
the respective JBS issue to P3, withdrawing this PR and creating a new PR that
targets openjdk/jdk20 after it has been created (which should be within hours
from now).
make/jdk/src/classes/build/tools/taglet/SealedGraph.java line 207:
> 205: private static String relativeLink(TypeElement rootNode,
> TypeElement node) {
> 206: var backNavigator =
> rootNode.getQualifiedName().toString().chars()
> 207: .filter(c -> '.' == c)
"Yoda conditions" are usually frowned upon. Can `c` be `null`?
-
PR: https://git.openjdk.org/jdk/pull/11580
