Hannes,
Nice follow up.
I'd still like to see us support generic types in references, where we
link the raw type and type arguments separately, but that can be done
separately. FWIW, we do have instances in the JDK docs where people try
to do that, and right now the hack workarounds often cause bad HTML
(where the type argument becomes an illegal HTML tag!)
OK for this round, fixing the use of arrays in references.
-- Jon
On 11/20/2018 01:14 AM, Hannes Wallnöfer wrote:
Thanks Jon. Yes, the code makes more sense this way, but it has a few
additional implications.
First, it made me realize we did the same dual check/error message for generic
types, and it turns out that non-existing generic types were untested and
caused a NPE in JavacTrees#attributeDocReference. I fixed that and added a test
for it.
However, with getElement no longer called for type parameterised and array
types, the fixes in JavacTrees#attributeDocReference are no longer covered by
the doclint test. That doclint test now only checks whether the right error
messages are generated, so I had to add a javadoc test to cover the changes in
JavacTrees#attributeDocReference.
New webrev with additional fix and tests:
http://cr.openjdk.java.net/~hannesw/8200432/webrev.03/
Hannes
Am 19.11.2018 um 20:23 schrieb Jonathan Gibbons <[email protected]>:
... but still call super.visitReference
-- Jon
On 11/19/18 11:21 AM, Jonathan Gibbons wrote:
My bad. You should not attempt to call getElement if you've reported errors. Put an
"else" around the latter part of the method, or else "return" after reporting
each error.
-- Jon
870 if (sig.contains("<") || sig.contains(">")) {
871 env.messages.error(REFERENCE, tree,
"dc.type.arg.not.allowed");
} else if (isArrayType(sig)) {
env.messages.error(REFERENCE, tree,
"dc.array.type.not.allowed");
} else {
873 Element e = env.trees.getElement(getCurrentPath());
874 if (e == null)
875 env.messages.error(REFERENCE, tree,
876 isArrayType(sig) ? "dc.array.type.not.allowed" :
"dc.ref.not.found");
}
877 return super.visitReference(tree, ignore);
On 11/19/18 11:14 AM, Hannes Wallnöfer wrote:
That would have been my preferred solution as well, but it generates a second
error message for arrays, because getElement() also returns null for arrays.
ReferenceTest.java:68: error: array type not allowed here
* {@link java.lang.String[]}
^
ReferenceTest.java:68: error: reference not found
* {@link java.lang.String[]}
^
Hannes
Am 19.11.2018 um 19:39 schrieb Jonathan Gibbons <[email protected]>
:
That's better, it is slightly strange/confusing to be referring to sig after
trying to get an element.
Here's a slightly cleaner suggestion. This also sets up the possibility of
checking for primitive types in the same manner.
870 if (sig.contains("<") || sig.contains(">")) {
871 env.messages.error(REFERENCE, tree,
"dc.type.arg.not.allowed");
} else if (isArrayType(sig)) {
env.messages.error(REFERENCE, tree,
"dc.array.type.not.allowed"
);
}
872
873 Element e = env.trees.getElement(getCurrentPath());
874 if (e == null)
875 env.messages.error(REFERENCE, tree, "dc.ref.not.found");
-- Jon
On 11/19/2018 10:01 AM, Hannes Wallnöfer wrote:
Thanks Jon.
Actually I think replacing the regex with a short method is a very good idea,
so I uploaded a new webrev:
http://cr.openjdk.java.net/~hannesw/8200432/
Hannes
Am 16.11.2018 um 19:36 schrieb Jonathan Gibbons <[email protected]>
:
Hannes,
I'm OK to defer a discussion on enhancing {@link}, and to go with the proposed
solution for now, regex and all.
Your point about not supported more types is noted. While I might argue that we should
support "{@link String[]}", even I think that allowing {@link int} would be
going too far.
-- Jon
On 11/16/2018 02:10 AM, Hannes Wallnöfer wrote:
Thanks for the feedback, Jon!
I don’t understand the point of supporting more link types unless we have
something to link to. For arrays we can link to the component type, but there’s
no actual link target for the array of a specific type.
Regarding the regex: I agree they’re not a great thing to read and maintain. It
might be a good idea to replace that with a small method comparing the index of
square brackets with that of left parenthesis.
Hannes
Am 16.11.2018 um 02:42 schrieb Jonathan Gibbons <[email protected]>
:
I'm not wildly enthusiastic about this, because while it prevents the CCE, it seems
to be generally going in the wrong direction. We need to be looking at supporting
more signatures in {@link}, not restricting the set of supported signatures. While
it may seem silly to write {@link String[]} it does make sense to want to write
{@link List<String>}. In other words, we should accept type signatures that
contain possible multiple names and other punctuation, just as we can write {@link
Object#equals(Other} and have it do the right thing.
I also note the use of a regular expression that is complicated enough for
Sundar to suggest that you use a comment. I would refer you to
http://regex.info/blog/2006-09-15/247
-- Jon
On 11/14/2018 06:43 PM, Sundararajan Athijegannathan wrote:
Updated webrev looks good!
-Sundar
On 14/11/18, 8:25 PM, Hannes Wallnöfer wrote:
Thanks, Sundar.
I uploaded a new webrev with a comment:
http://cr.openjdk.java.net/~hannesw/8200432/webrev.01/
Hannes
Am 13.11.2018 um 16:32 schrieb Sundararajan
Athijegannathan<[email protected]>
:
Looks good.
Minor nit: There could be a source comment for this pattern in Checker.java
+ private final static Pattern arrayPattern =
Pattern.compile("^[^\\(]+\\[]");
-Sundar
On 13/11/18, 6:52 PM, Hannes Wallnöfer wrote:
Please review:
Issue:
https://bugs.openjdk.java.net/browse/JDK-8200432
Webrev:
http://cr.openjdk.java.net/~hannesw/8200432/webrev.00/
Thanks,
Hannes
