On Wed, 22 Sep 2021 16:11:07 GMT, Jonathan Gibbons <j...@openjdk.org> 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:
>
> address preliminary review comments
src/jdk.compiler/share/classes/com/sun/tools/javac/tree/DCTree.java line 161:
> 159: }
> 160:
> 161: switch (getKind()) {
I can see that you changed this switch while moving it from JavacTrees.
src/jdk.compiler/share/classes/com/sun/tools/javac/tree/DCTree.java line 174:
> 172: case IDENTIFIER -> {
> 173: DCIdentifier ident = (DCIdentifier) this;
> 174: return ident.pos + ident.name.length();
While in JavacTrees, this logic contained some special handling for
`names.error`. What was that about?
src/jdk.compiler/share/classes/com/sun/tools/javac/tree/DCTree.java line 177:
> 175: }
> 176:
> 177: case AUTHOR, DEPRECATED, HIDDEN, PARAM, RETURN, SEE, SERIAL,
> SERIAL_DATA, SERIAL_FIELD, SINCE, THROWS, UNKNOWN_BLOCK_TAG, VERSION -> {
You added HIDDEN and merged PARAM into this case of block tags. Should we also
add PROVIDES and USES? If so, then we should add respective tests.
src/jdk.compiler/share/classes/com/sun/tools/javac/tree/DCTree.java line 192:
> 190: case ENTITY -> {
> 191: DCEntity endEl = (DCEntity) this;
> 192: return endEl.pos + endEl.name.length() + 2;
A similar question about `names.error`.
-------------
PR: https://git.openjdk.java.net/jdk/pull/5510
