On Mon, 29 Jan 2024 23:34:24 GMT, Jonathan Gibbons <j...@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
