On Tue, 21 Sep 2021 22:20:07 GMT, Jonathan Gibbons <[email protected]> wrote:
>> Please review a medium size update/overhaul to the way that positions and
>> diagnostic positions are handled in the `DocTree`/`DCTree` world.
>>
>> ## Previously ...
>>
>> Previously, most `DCTree` nodes only had an explicit "position" (`pos`).
>> Start and end positions were provided by `DocSourcePositions`, but these
>> were not directly available in tree nodes, because of the lack of access to
>> the `DocSourcePositions` class. (In contrast, `JCTree` nodes do have
>> position info, via static methods in `TreeInfo`.) The one notable exception
>> to these guidelines was `DCErroneous` which (by analogy with `JCTree`)
>> directly implemented `JCDiagnostic.DiagnosticPosition` but that was an
>> arguably bad implementation because the positions were relative to the start
>> of the comment, and not the start of the file. This did not show up in code
>> and tests, because diagnostics related to `DocTree` nodes used `DCTree.pos`
>> which returned a `SimpleDiagnosticPosition` referencing just the start
>> position -- at least in part because more specific information was not
>> easily available.
>>
>> ## Now ...
>>
>> All `DCTree` nodes have 4 positions, 3 publicly available.
>> * the position of the first unique character, `pos`
>> * the starting position of the range of characters, `getStartPosition()`
>> * the "preferred" position in the range, used to position the caret in
>> diagnostic messages, `getPreferredPosition()`
>> * the end position of the range of characters, `getEndPosition()`.
>> These are all relative to the beginning of the comment text, and are
>> converted to file positions as needed.
>>
>> Code to implement the end position is moved from `JavacTrees` to `DCTree`.
>> It's still a switch on kind, but could reasonably be converted to using
>> overriding methods.
>>
>> `DCErroneous` no longer implements `JCDiagnostic.DiagnosticPosition`. the
>> constructor methods to create a diagnostic are removed, in favor of passing
>> in a correctly-formed diagnostic.
>>
>> `DCTree` has a new improved `pos(DCDocComment)` method which now uses the
>> new start/pref/end position methods.
>>
>> `DocCommentParser.ParseException` and the `erroneous` method now take an
>> optional "pos" parameter to allow the position of an error to be more
>> accurately specified.
>>
>> ## Testing ...
>>
>> Up until the point at which `DCTree.pos` was modified, all tests passed
>> without change. When `DCTree.pos()` was modified, a few (3) doclint tests
>> starting failing, demonstrating a latent reliance of the old form of
>> `DCTree.pos()`. These tests are updated.
>>
>> Rather than write a single new test, the existing `DocCommentTester` class
>> is updated to include a new `Checker` which, generally, checks the "left to
>> right" nature of all positions in a doc comment tree and its subtrees. This
>> leverages all the existing good and bad test cases in the
>> `tools/javac/doctree` directory, which therefore all get tagged with the bug
>> number for this issue.
>>
>> ## Behavior ...
>>
>> Apart from fixing generally bad behavior, there is one other tiny behavioral
>> change. For an empty `DocCommentTree` the ending position is now the same at
>> the starting position, and not `NOPOS`. This was triggered by the new code
>> in `DocCommentTester`, but which affected one `jshell` test, which started
>> getting `StringIndexOutOfBounds` exception. This is minimally fixed for now,
>> but the code in question would arguably be better to use the new
>> comment-based positions, rather than the file-based positions. But that
>> seems out of scope for this work. Also worth investigating is the method
>> `JavacTrees.getDocCommentTree(FileObject fileObject)` which uses a fixed
>> offset of 0 (JavacTrees, line 1052) when creating doc comments for HTML
>> files.
>
> Jonathan Gibbons has updated the pull request incrementally with one
> additional commit since the last revision:
>
> revert unrelated debug edit
As this PR contains a lot clean-up changes and improvements, it will take me
extra time to review it. Here's the first batch of comments.
src/jdk.compiler/share/classes/com/sun/tools/javac/api/JavacTrees.java line 269:
> 267: DCTree dcTree = (DCTree) tree;
> 268: if
> (dcComment.getSourcePosition(dcTree.getEndPosition()) == 0) {
> 269: System.err.println("WARNING: comment:" +
> dcComment.comment + ": " + dcTree.getKind() + " end:" +
> dcTree.getEndPosition() + " result: " +
> dcComment.getSourcePosition(dcTree.getEndPosition()));
This looks like a leftover debugging aid.
src/jdk.compiler/share/classes/com/sun/tools/javac/parser/DocCommentParser.java
line 971:
> 969: /**
> 970: * Creates an {@code ErroneousTree} node, for a range of text
> starting at a given position,
> 971: * ending at the last non-white character before the current
> position,
In this file and in JDK overall, such characters as predominantly called
"non-whitespace".
src/jdk.compiler/share/classes/com/sun/tools/javac/parser/DocCommentParser.java
line 985:
> 983: /**
> 984: * Creates an {@code ErroneousTree} node, for a range of text
> starting at a given position,
> 985: * ending at the last non-white character before the current
> position,
ditto
src/jdk.compiler/share/classes/com/sun/tools/javac/parser/DocCommentParser.java
line 986:
> 984: * Creates an {@code ErroneousTree} node, for a range of text
> starting at a given position,
> 985: * ending at the last non-white character before the current
> position,
> 986: * and with a given preferred position
Add full stop.
src/jdk.compiler/share/classes/com/sun/tools/javac/parser/ReferenceParser.java
line 124:
> 122: moduleName = switch (slash) {
> 123: case -1 -> null;
> 124: case 0 -> throw new ParseException(0,
> "dc.ref.syntax.error");
There's a slight change in error output. Although it is likely insignificant, I
felt I should note it. For example, compare the error output for `{@link
//java.lang.Object}`.
src/jdk.compiler/share/classes/com/sun/tools/javac/tree/DCTree.java line 74:
> 72: * </ul>
> 73: *
> 74: * All are values relative to the beginning of the
Transposition error? "All values are relative..."
src/jdk.compiler/share/classes/com/sun/tools/javac/tree/DCTree.java line 88:
> 86: /**
> 87: * The position of the first character that is unique to this node.
> 88: * It is normally set by the methods in {@link DocTreeMaker}.
Since this change also includes cleanup, we might as well fix "the the" and
"non-white space" at DocTreeMaker.java:699.
src/jdk.compiler/share/classes/com/sun/tools/javac/tree/DCTree.java line 161:
> 159: }
> 160:
> 161: switch (getKind()) {
This switch might become even better after "Pattern Matching for switch" has
been integrated (currently in preview, JEP 406).
src/jdk.compiler/share/classes/com/sun/tools/javac/tree/DCTree.java line 265:
> 263: * {@return a diagnostic position based on the positions in a
> comment}
> 264: *
> 265: * The positions are lazily converted to file-based positions as
> needed.
"lazily" somewhat duplicates "as needed", no?
test/langtools/tools/javac/doctree/SerialTest.java line 26:
> 24: /*
> 25: * @test
> 26: * @bug 7021614 8273244 8273244
Duplicating bug IDs.
-------------
PR: https://git.openjdk.java.net/jdk/pull/5510
