On Mon, 17 May 2021 20:40:18 GMT, Jonathan Gibbons <[email protected]> wrote:
>> Please review a change to overhaul the javadoc support for diagnostics to
>> better leverage the support available in javac. This includes the ability
>> for all javadoc diagnostics to show a "source line and caret" to indicate
>> the position of a reported issue. As a side-effect, it normalizes the
>> formatting of javadoc messages, to be consistent with javac messages.
>>
>> The primary changes are in the javadoc `Messager` class, and are primarily
>> focussed "downward" on the internal use of the javac `Log.report` method,
>> which is the nexus for reporting methods. There is additional cleanup that
>> could be done in `Messager` in the API it provides to clients, but (for the
>> most part) that is not done in this work.
>>
>> Additional changes are done to facilitate writing a test for this work, and
>> reflect the current shortcomings of the existing `Doclet` API. Most notably:
>> * changes in `Utils` to allow a user-defined taglet to override a built-in
>> taglet
>> * changes in `TagletManager` and `Workarounds` to allow a user-defined
>> taglet to access internal API, to workaround API that would be useful to
>> provide on `StandardDoclet`
>>
>> There are a few minor specific cleanup changes in code style and/or improved
>> comments.
>>
>> There is one primary new test, `TestDiagsLineCaret.java` which exercises
>> different kinds of diagnostics at different positions in a file, to verify
>> that the source line and a caret are produced as appropriate.
>>
>> There are additional test changes triggered by the slight change in the
>> format of error messages. Most notably, prefixes like `error -` and
>> `warning -` become `error:` and `warning:`.
>
> Jonathan Gibbons has updated the pull request incrementally with one
> additional commit since the last revision:
>
> minor cleanup
I will continue reviewing this change. Meanwhile, please update copyright years
and address other nits below.
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/WorkArounds.java
line 543:
> 541: * Returns whether or not to permit dynamically loaded components to
> access
> 542: * part of the javadoc internal API. The flag is the same (hidden)
> compiler
> 543: * option that allows javac plugins and annotation processors to
> javac internal
Do you use "allow x to y" here in the sense of "letting x into y"? If so,
consider using "allow x into y" instead of "allow x to y". Otherwise it might
sound like there's a verb missing: allow to (access? enter?) javac internal
API. My non-native English speaker's two cents.
src/jdk.javadoc/share/classes/jdk/javadoc/internal/tool/Messager.java line 67:
> 65: * of diagnostic messages and to avoid code duplication.
> 66: *
> 67: * The class is a subtype of javac's Log, and is primarily a transducer
> between
Would "adapter" be more suitable than "transducer"?
src/jdk.javadoc/share/classes/jdk/javadoc/internal/tool/Messager.java line 92:
> 90: * <li>{@code Diagnostic.Kind} -- maps to {@code DiagnosticType} and
> {@code Set<DiagnosticFlag>}
> 91: * <li>{@code Element} -- maps to {@code DiagnosticSource and {@code
> DiagnosticPosition}
> 92: * <li>{@code DocTreePath} -- maps to {@code DiagnosticSource and
> {@code DiagnosticPosition}
Close `@code` tags.
src/jdk.javadoc/share/classes/jdk/javadoc/internal/tool/Messager.java line 96:
> 94: *
> 95: * The javac layer deals primarily in pre-localized (key, args) pairs,
> 96: * while the javadoc layer, especially the {@code Reporter} interface,
> deals in localized strings.
If it is not a typo, consider changing "deal in" to something simpler.
src/jdk.javadoc/share/classes/jdk/javadoc/internal/tool/Messager.java line 125:
> 123: private ToolEnvironment toolEnv;
> 124:
> 125: /** The utility class to access the positions of items in doc
> comments., */
Remove that trailing comma.
src/jdk.javadoc/share/classes/jdk/javadoc/internal/tool/Messager.java line 332:
> 330:
> 331: /**
> 332: * Print a "notice" message.
Add "s" to "print" for consistency with doc comment of other methods in this
class.
src/jdk.javadoc/share/classes/jdk/javadoc/internal/tool/Messager.java line 448:
> 446: * such as "Generating class ...". Therefore, for now, we detect
> and report those
> 447: * messages directly. (A better solution would be to expose access
> to the output
> 448: * and error streams via {@code DocletEnvironment}.
Sentence: either remove "(" or add ")".
test/langtools/jdk/javadoc/doclet/testDiagsLineCaret/MyTaglet.java line 127:
> 125: @Override
> 126: public Void visitEntity(EntityTree node, Diagnostic.Kind k) {
> 127: if (node.getName().contentEquals("#x1f955")) {
Is this... a carrot (🥕)?
test/langtools/jdk/javadoc/doclet/testMissingComment/TestMissingComment.java
line 115:
> 113: javadoc("-d", base.resolve("api").toString(),
> 114: "-Xdoclint:missing",
> 115: "--no-platform-links",
More of those... sigh.
test/langtools/jdk/javadoc/tool/MaxWarns.java line 78:
> 76: StringWriter sw = new StringWriter();
> 77: PrintWriter pw = new PrintWriter(sw);
> 78: String[] args = { "-Xdoclint:none", "--no-platform-links", "-d",
> "api", f.getPath() };
Argh...
-------------
Changes requested by prappo (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/4074
