Generally excellent. Thanks for finally addressing this long-standing wish.
I did some research (archaeology) on the fuzzyMatch issue. It looks like
a lot of the code in this area was imported from the old JDK 8-era
standard doclet, as evidenced by a number of now-obsolete/broken
references like:
@see com.sun.tools.javadoc.ClassDocImpl
fuzzyMatch itself seems to have been new in the JavacTrees code,
superseding some equally fuzzy code in the old doclet, which was not
appropriate to import. Anyway, we moved all this analysis onto a better
foundation, so I'm pleased to see the old fuzzy code go away. You might
want to consider removing the broken/obsolete `@see` references,
although in this case they were actually useful to point back to the
origin of the code, in JDK 8.
In LinkInfoImpl, I guess VARIABLE_ELEMENT_COPY was unused, but I guess I
was surprised that a couple of other _COPY names were not deleted,
presumably because they were used. Anyway, it's the difference in
treatment that I find curious.
Tiny Nit: In CommentHelper:196, `doctrees` would be better written as
`docTrees`.
This work probably merits a Release Note.
-- Jon
On 4/8/20 9:21 AM, Hannes Wallnoefer wrote:
Jon,
I uploaded a new webrev for this issue:
http://cr.openjdk.java.net/~hannesw/8177280/webrev.04/
Changes include:
- updated test to match changes in HTML/CSS
- added @since 15 tag to new DocTrees method
- allow parameterised types in signatures of method links
The last item requires a bit more explanation.
It is now possible to link to an executable member specifying
parameterised types in the signature, e.g.:
@see #getItem(List<String>,int)
In contrast to links to parameterized types which generate a link to
each type component the member link will generate a single link to the
method with the full signature as link text. This is consistent with
the current behaviour of @see/@link method linking where parameter
types are not separate links but part of the main link.
The support this feature I slightly changed the implementation of
private #hasParameterTypes method in JavacTrees to first compare the
exact parameter types to a member’s signature before comparing the
erased types.
It is possible to specify subtypes in the reference signature, i.e. a
method with parameter (? extends Number) can be linked with the exact
declared signature or with valid subtypes such as (Integer). I’m not
sure if this is desired, but it seems reasonable and potentially
useful to me.
I also removed the fuzzyMatch code in JavacTrees signature comparison.
The fuzzyMatch method allowed to use erraneous types such as
List<NonExistingClass> in signature lookups. I’m not totally whether
some of the behaviour in fuzzyMatch is still desired, but it was very
old code and removal didn’t trigger any test failures.
Hannes
Am 03.03.2020 um 22:54 schrieb Jonathan Gibbons
<jonathan.gibb...@oracle.com <mailto:jonathan.gibb...@oracle.com>>:
The new method on DocTrees does not have @since and needs a CSR.
I ran the new test to confirm all parts of the link. Nice, although
there are lots of "external link" icons.
The test has worn out a bit, because of independent changes. I sent
you (separately) a patchy for the new test. With that patch, the test
works for me.
OK to push if you patch the test and when you have got the
appropriate CSR approvals.
-- Jon
On 02/27/2020 02:56 AM, Hannes Wallnöfer wrote:
Thanks for the review, Jon.
New webrev: http://cr.openjdk.java.net/~hannesw/8177280/webrev.03/
Comments inline below.
Am 25.02.2020 um 00:11 schrieb Jonathan Gibbons
<jonathan.gibb...@oracle.com <mailto:jonathan.gibb...@oracle.com>>:
HtmlDocletWriter:1044
Changing RawHtml to StringContent is a significant behavioral
change. It's not explicitly stated in the doc comment spec whether
the text may contain HTML, but it is reasonable to infer from the
descriptions of {@code} and {@literal} that it may be HTML content.
Is this change necessary?
You are right, that change was mostly driven by personal preference,
which is not a good guideline.
I reversed it in the new webrev.
CommentHelper ... clever changes, I think ;-)
In the new test, the direct/explicit use of
https://docs.oracle.com/en/java/javase/12/docs/api/ may cause
issues later, in that "sanity scripts" may discover the string and
wonder if it is an out of date reference. At a minimum, the use of
the URL should be significantly commented in the test. But,
despite the comment on the `testValidLinks` method, I don't think
it actually checks that the links exist (as compared to 404-Not
Found). Since you are using -linkoffline, it may be sufficient to
just do a global replace of
https://docs.oracle.com/en/java/javase/12/docs/api
to
http://example.com/docs/api
or something like that.
I replaced the URLs as suggested, indeed the test is passing.
Hannes
-- Jon
On 02/18/2020 03:26 AM, Hannes Wallnöfer wrote:
I have updated the patch to recent changes in CommentHelper, no
changes otherwise.
http://cr.openjdk.java.net/~hannesw/8177280/webrev.01/
Hannes
Am 07.02.2020 um 19:34 schrieb Hannes Wallnöfer
<hannes.wallnoe...@oracle.com>:
Please review:
JBS: https://bugs.openjdk.java.net/browse/JDK-8177280
Webrev: http://cr.openjdk.java.net/~hannesw/8177280/webrev.00/
As I said previously there are some things in this patch I’m
unsure or not quite happy about.
Some notes:
- While the implementation of the new DocTrees method is quite
simple, I’m not sure if I should install a new
Log.DeferredDiagnosticHandler for the runtime of the method, like
the attributeDocReference method in the same class does.
- In HtmlDocletWriter.seeTagToContent I changed the handling of
the tag text to escape HTML characters if no label is specified.
This causes „<„ and „>“ to be displayed in the browser instead of
being interpreted as HTML tag if a generic link target can’t be
resolved. This is potentially problematic since @see and @link
can contain plain HTML content, but I think it’s ok since those
cases are handled further up in HtmlDocletWriter.seeTagToContent.
- That same method in HtmlDocletWriter contained some never-used
code (dependent on the BaseConfiguration.backwardCompatibility
flag which is always true) to use the fully qualified class name
for links in some cases. I removed that code and the field in
BaseConfiguration since it adds to that already long-winding method.
- Setting the label on a LinkInfoImpl basically disables
rendering of type arguments. While I understand the rationale
behind this, it might still be useful to set the label for the
main link of a generic type. I’ve tried to remove the
restriction, but ran into a lot of problems (i.e. failing tests)
in other places. Since the current behaviour does what we need I
decided to not change this.
- There was a potential infinite recursion in
LinkFactoryImpl.getTypeParameterLinks caused by following the
type parameters of linkInfo.typeElement when linkInfo.type is
set. The problem was that linkInfo.typeElement may be set to the
owner of linkInfo.type, and the solution is to only follow that
path if linkInfo.type is null.
- I removed two unused LinkInfo Kind enum values.
- I CommentHelper there were previously two and now three
Visitors to extract ReferenceTrees, so I added a common base
class called ReferenceDocTreeVisitor.
- As an completely unrelated cleanup I removed the unused javafx
field in IndexBuilder.
Thanks,
Hannes
