RFR: JDK-8286101: Support formatting in @value tag

[Jonathan Gibbons]

On Mon, 16 May 2022 10:07:18 GMT, Pavel Rappo <prappo at 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