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 >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>>
