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

[Pavel Rappo]

On Mon, 29 Jan 2024 23:34:24 GMT, Jonathan Gibbons <jjg at openjdk.org> 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