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
<[email protected] <mailto:[email protected]>> 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
