> 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: fix handling of PROVIDES and USES add new StartEndPosChecker for DocCommentTester add new CoverageTest, to ensure DocCommentTester tests provide full coverage ------------- Changes: - all: https://git.openjdk.java.net/jdk/pull/5510/files - new: https://git.openjdk.java.net/jdk/pull/5510/files/cf6d3b8e..14c29bdc Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5510&range=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5510&range=02-03 Stats: 263 lines in 3 files changed: 259 ins; 1 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/5510.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5510/head:pull/5510 PR: https://git.openjdk.java.net/jdk/pull/5510
