Jon,
The idea of the fix looks reasonable. Since I'm new to javadoc, I have more
questions than would
probably be expected during a typical review.
1. Could you help me understand the asymmetry in BaseTaglet?
public boolean accepts(DocTree tree) {
return (tree.getKind() == DocTree.Kind.UNKNOWN_BLOCK_TAG
&& tagKind == DocTree.Kind.UNKNOWN_BLOCK_TAG)
? ((UnknownBlockTagTree) tree).getTagName().equals(name)
: tree.getKind() == tagKind;
}
Why does this method check the pair (kind, name) for the case of
UNKNOWN_BLOCK_TAG, but only checks
the name in any other case, including the case where kind == UNKNOWN_INLINE_TAG?
2. I think we could use contravariant generic parameter in the predicate in
Utils:
- public List<? extends DocTree> getBlockTags(Element element,
Predicate<DocTree> filter) {
+ public List<? extends DocTree> getBlockTags(Element element, Predicate<?
super DocTree> filter) {
On a related note. I noticed that com.sun.source.doctree.DocCommentTree uses
bounded wildcards in
the return types of its methods (e.g. List<? extends DocTree>). From a caller's
perspective,
List<DocTree> has the same usability as List<? extends DocTree>, if the caller
agrees to only
consume the elements from the list and to never add them there. Which I think
was the intent here.
Unless I'm mistaken, the only reason to use bounded wildcards in the return
types is to design for
covariant returns in subtypes. For example,
interface MyDocTree extends DocTree {
...
List<? extends BlockTagTree> getBlockTags();
...
}
We don't seem to have those in javadoc. At the same time we're paying a small
boilerplate fee for
this--unused flexibility--each and every time one of those methods is called.
What's worse is that
this has a ripple effect, causing long generified lines to appear far beyond
those methods' call
sites.
I guess what I'm saying is that we could look into simplifying that on the
javadoc side (not the
javac side).
3. Why did you remove @SuppressWarnings("fallthrough") from
CommentHelper.getTagName?
4. Am I right saying that there are no JavaFX-specific taglets? And that is the
reason why the
"propertyDescription" and "defaultValue" tags are processed on the spot. At the
same time the
following constructor was left intact:
BaseTaglet(String, boolean, Set<Taglet.Location>)
Is it also for the sake of JavaFX? I wonder if we should spend some time later
to hide everything
JavaFX-related to a neat class rather than having it sprinkled all over the
place.
Thanks for cleaning up the code along the way.
-Pavel
> On 14 Dec 2019, at 02:17, Jonathan Gibbons <[email protected]>
> wrote:
>
> Please review a moderately simple cleanup to the implementation(s) of
> Utils.getBlockTags.
>
> The existing code is unnecessarily string-oriented, and can be improved by
> better leveraging DocTree.Kind, especially by updating each subtype of
> BaseTaglet to know its associated DocTree.Kind.
>
> The core of the fix is Utils, with additional support in BaseTaglet and
> SimpleTaglet. The other changes are derivative changes using the new API.
>
> There are more changes possible in this (general) area. For example, there
> are similar methods such as Utils.hasBlockTag, and methods like
> CommentHelper.getTagName. At a minimum, it may be reasonable to co-locate all
> these methods in a new "Tags" utility class, but it is also worth
> investigating what additional simplifications can be made. But for now, this
> is a good checkpoint.
>
> The old code accidentally covered up a pre-existing bug, which was exposed in
> the replacement code. The old code did not return @uses and @provides from
> getBlockTags, and so they did not not to be skipped as part of the main
> comment in ModuleWriterImpl. Now they are returned by getBlockTags, and so
> need to be skipped in TagletWriter.
>
> This is all cleanup with no changes in the generated output. There are no new
> tests and no changes needed to any existing tests. A full comparison against
> a reference JDK was done with the standard JDK docs (make docs) and with all
> the output from all the jtreg javadoc tests.
>
> -- Jon
>
> JBS: https://bugs.openjdk.java.net/browse/JDK-8235947
> Webrev: http://cr.openjdk.java.net/~jjg/8235947/webrev.00/
>
