On Fri, 1 Oct 2021 00:37:19 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: > > improve comments in new tests Changes requested by prappo (Reviewer). src/jdk.compiler/share/classes/com/sun/tools/javac/tree/DCTree.java line 225: > 223: } > 224: > 225: return Position.NOPOS; This method should return Diagnostic.NOPOS. I'm concerned with mixing com.sun.tools.javac.util.Position.NOPOS and javax.tools.Diagnostic.NOPOS. Although they currently have the same int value, they are of different types, used in different contexts, and not guaranteed to remain equal in the future. We might need to do something about it. ------------- PR: https://git.openjdk.java.net/jdk/pull/5510
