RFR: JEP 359-Records: javadoc code

Tue Nov 5 21:28:52 UTC 2019

Hi Kumar,

Thanks for the feedback. I'll be posting another webrev shortly.

Let me respond here to some of the issues you raise.

> + // Special case to allow '{@link ...}' to appear in the string.
> + // A less general case would be to detect literal use of Object.equals
> + // A more general case would be to allow access to DocCommentParser 
> somehow
> Yes the church vs. state rule, I don't quite understand the problem we 
> are trying solve with this code.

I'm not sure this is the church vs. state rule.  The problem I am 
solving is that I want the localized generated comment to be able to 
refer to the program elements like Objects.equals. Without any special 
handling, using `{@link}` in a resource in the resource file will be 
treated as literal text and will appear in the generated text exactly as 
written. The special handling I provided was to explicitly detect 
`{@link} strings and to replace them in the synthesized DocCommentTree 
with the appropriate LinkTree node.  As such, it's like a custom 
micro-parser for `{@link}' in the resource string.  As I mentioned in 
the comment, the alternatives are to make the code be even more 
specialized and detect/replace an otherwise plain string of 
'Objects.equals` or whatever, or at the other extreme, to create and use 
a full DocCommentParser on the resource string to create the DocCommentTree.

>
> With the Element ordering changes (ElementComparator) I thought this 
> might cause failures
> in TestOrdering, I guess the previous ordering are being preserved, 
> but there are no  Records
> elements in this test ?
The change was mostly just a simple/trivial refactoring from using a 
custom static Map to get the ordering constant, to using a switch 
statement.  I assumed the existing tests would cover the change. You are 
right that we could update the existing ordering test to include records.

-- Jon

On 11/02/2019 08:12 AM, Kumar Srinivasan wrote:
> Hi Jon,
>
> Firstly I must commend the javadoc.next project for updating 
> javadoc/doclet to use jx.l.m and
> other improvements to the doc comments management, this has made it 
> relatively easy for javadoc
> to implement new language features, such as this. Having said that, 
> the changes are looking
> great, and the langtools stalwarts have done due diligence.
>
> Here are some minor review comments, I don't need to see another revision.
>
> Default webrev:
>
>
>     src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/builders/ClassBuilder.java
>
> Likely search/replace
> - * Keep track of whether or not this typeElement is an record.
> + * Keep track of whether or not this typeElement is a record.
>
> - // only one canonical constructor; no longer need to keep looking
> + // only one canonical constructor; no need to keep looking
>
> - TypeMirror objectType = 
> utils.elementUtils.getTypeElement("java.lang.Object").asType();
> + TypeMirror objectType = utils.getObjectType();
> There is a symbol table cache maintained in Utils, for performance 
> improvements.
>
>
>     src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/util/Utils.java
>
> Nice simplifications to modifiersToString. <blush>
>
> With the Element ordering changes (ElementComparator) I thought this 
> might cause failures
> in TestOrdering, I guess the previous ordering are being preserved, 
> but there are no  Records
> elements in this test ?
>
>
>     src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/CommentUtils.java
>
> + private void add(List<DocTree> contents, String resourceKey) {
> + // Special case to allow '{@link ...}' to appear in the string.
> + // A less general case would be to detect literal use of Object.equals
> + // A more general case would be to allow access to DocCommentParser 
> somehow
> Yes the church vs. state rule, I don't quite understand the problem we 
> are trying solve with this code.
> + * name is to be inserted. T
> + * @param key the resource key for the description
> Looks like we have a hanging "T".
>
> One last thing, was the doc comparator run ? The reason I ask,  there 
> seems to be unrelated
> improvements ex: Utils::modifiersToString etc, not sure if regressions 
> are lurking in the output.
>
> Best,
>
> Kumar Srinivasan
>
> PS: I will have to go into radio silence for some time.
>
> On Wed, Oct 30, 2019 at 4:54 PM Jonathan Gibbons 
> <jonathan.gibbons at oracle.com <mailto:jonathan.gibbons at oracle.com>> wrote:
>
>     Please review a moderately small update to the proposed support for
>     records in javadoc.
>
>     The primary change is to include record components in the
>     signature of a
>     record displayed near the top of the page.
>
>     In addition, a "combo test" is added into TestRecordTypes.java to
>     verify
>     the presence or absence of annotations in various places in the
>     generated page for a record, depending on the `@Target` of the
>     annotations.
>
>     Finally, there are some small cosmetic changes, and the supporting
>     files
>     for some previously published examples.
>
>     Two webrevs are provided.
>
>     The first is a cumulative webrev of the modified javadoc source
>     and test
>     files, compared against the default branch of the amber repo (i.e.
>     the
>     state of the jdk/jdk repo)
>     http://cr.openjdk.java.net/~jjg/amber-records/webrev.default/
>     <http://cr.openjdk.java.net/%7Ejjg/amber-records/webrev.default/>
>
>     The second is a "delta webrev" of the recently modified javadoc
>     source
>     and test files, compared against the tip of the records branch of the
>     amber repo.
>     http://cr.openjdk.java.net/~jjg/amber-records/webrev.tip/
>     <http://cr.openjdk.java.net/%7Ejjg/amber-records/webrev.tip/>
>
>     Also, the sets of examples are updated, showing examples linked
>     and not
>     linked to JDK API docs
>     http://cr.openjdk.java.net/~jjg/amber-records/examples/api-with-link/
>     <http://cr.openjdk.java.net/%7Ejjg/amber-records/examples/api-with-link/>
>     http://cr.openjdk.java.net/~jjg/amber-records/examples/api-no-link/
>     <http://cr.openjdk.java.net/%7Ejjg/amber-records/examples/api-no-link/>
>
>     Finally, I note a curiosity, arising from the proposed spec. This is
>     the first occurrence that I can think of in which an item that is
>     syntactically necessary in the source code does /not/ show up in the
>     same place in the generated documentation.  In general, in previous
>     instances where the documented signatures differ from the source
>     code,
>     the difference has been the addition of default or mandated elements.
>     Here, the presence of an annotation on the declaration of a record
>     component in source code may not show up in the corresponding
>     place in
>     the documented signature, depending on the specified @Target for the
>     annotation. I'm not saying that's wrong, but it is curious, and
>     may need
>     explaining to some.
>
>     -- Jon
>
>     JEP 359: https://openjdk.java.net/jeps/359
>