On Mon, 16 May 2022 10:07:18 GMT, Pavel Rappo <pra...@openjdk.org> wrote:
> So far the idea looks okay. My feedback is purely on its implementation.
>
> I'm concerned about backward compatibility. If we believe that it is unlikely
> that javadoc can be configured to use a custom implentation of
> `DocTreeFactory`, then the proposed implementation looks okay. Otherwise,
> consider a case where a new javadoc tool reads old doc comments. Do we expect
> it to fail with `UnsupportedOperationException` for every `value` tag in
> those doc comments?
>
> My inline comments below are about that latter case; ignore them if you don't
> think it's an issue.
1. It is very unlikely that you could/would configure mismatched versions of
the jdk.javadoc and jdk.compiler modules.
2. Note that a new javadoc tool can read old doc comments (because the format
is optional) and should not throw exceptions. The problem will occur if a new
javadoc tool uses and old `DocCommentParser` in a mismatched module.
> src/jdk.compiler/share/classes/com/sun/tools/javac/parser/DocCommentParser.java
> line 1605:
>
>> 1603: if (ch == '}') {
>> 1604: nextChar();
>> 1605: return m.at(pos).newValueTree(format, ref);
>
> If `format == null` call `newValueTree(ref)`; otherwise call
> `newValueTree(format, ref)`.
I guess I don't think it's an issue but I'm OK with the proposed next form.
> src/jdk.compiler/share/classes/com/sun/tools/javac/tree/DocTreeMaker.java
> line 482:
>
>> 480:
>> 481: @Override @DefinedBy(Api.COMPILER_TREE)
>> 482: public DCValue newValueTree(ReferenceTree ref) {
>
> Revert the implementation of this method.
That's not necessary, because we're in the private implementation world here.
The important change you're suggesting is to the methods in the
`DocTreeFactory` interface.
> src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/ValueTaglet.java
> line 112:
>
>> 110: text = String.format(configuration.getLocale(), f,
>> field.getConstantValue());
>> 111: } catch (IllegalFormatException e) {
>> 112: messages.warning(holder,
>
> Doesn't CSR say that it is an *error*?
The CSR doesn't say, but I changed it to an error anyway.
> test/langtools/tools/javac/doctree/ValueTest.java line 108:
>
>> 106: */
>> 107:
>> 108: /**
>
> Consider adding the following cases:
> * erroneous format
> * valid format and reference followed by the trailing "junk"
done
-------------
PR: https://git.openjdk.java.net/jdk/pull/8565
