On 12/16/19 11:45 AM, Pavel Rappo wrote:
Jon,
On 16 Dec 2019, at 19:00, Jonathan Gibbons <[email protected]> wrote:
On 12/16/2019 10:44 AM, Pavel Rappo wrote:
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?
The DocTree API has specific nodes, with a specific kind, for all "standard"
nodes, but the API also
has to cope with additional/unknown/non-standard nodes. These are (all)
handled by
UnknownBlockTagTree, which contains the name of the non-standard tag.
Thus, for standard tags, it is sufficient to check the kind; for non-standard
tags, represented by
instances of UnknownBlockTagTree, you need to check the name stashed in the
UnknownBlockTagTree
node.
Thanks, I got the idea. Still, I'll have to read the code further to understand
the behavior
specific to UNKNOWN_INLINE_TAG.
UNKNOWN_INLINE_TAG obviously exists for the same reason. Ideally, there
would just be UNKNOWN_TAG,
but for better or worse, we went down the path of supertypes for
BlockTagTree and InlineTagTree.
With 20/20 hindsight, it's not clear those supertypes have been very
helpful, compared to (say)
having methods "isInline" and "isBlock".
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) {
Since there are no useful/interesting supertypes of the DocTree interface, I
don't see that it is
necessary or helpful to use super-wildcards.
Fair enough. I think it was a knee-jerk reaction on my part. I usually add
things like this because
it costs nothing to the API client, but adds some extra comfort to user
experience.
... except that later on, you're also suggesting we avoid them ;-)
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).
a) we do use the ability to return lists of subtypes
Do we use it in the javadoc?
Probably only indirectly, in cases where we want to propagate results
from public API that does
use the wildcard. I know there have been times when I've tried to go
simple, and the compiler
has rejected the code.
In terms of code-style for internal API, I don't think it's necessary to
always use wildcards without
due consideration of the need and context.
b) this is a public API that now cannot reasonably be changed
Which API is that? I might not have expressed myself clearly. I'm talking about
the javadoc code
only. Not about the contents of the com.sun.source.doctree package in
jdk.compiler.
OK, sorry, misunderstood a bit, here.
Anyhow, I guess some of those overly verbose call sites could be treated with
"var".
Yeah, I haven't yet bought in to using "var" but that may just be a
Luddite reaction.
I find it messy/inconsistent when you have nearby code some of which can
use var
and some of which cannot.
3. Why did you remove @SuppressWarnings("fallthrough") from
CommentHelper.getTagName?
It was not required. It is a common misconception that multiple case-labels
are an instance
of fallthrough semantics. It is not. The fact that javadoc still builds, with
-Werror, and no
change to the compilation command is additional evidence that the annotation
was redundant.
Ideally, there ought to be a javac lint warning for unnecessary
@SuppressWarnings!
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>)
There are no standard JavaFX tags/taglets. The relationship between javadoc
and JavaFX is
long and somewhat sad. Early in the life of JavaFX, ~JDK8, the FX team added
code to javadoc
to handle their stuff. In 9 the world was improved such that JavaFX was
bundled/included
with JDK 9. Then, eventually, it got removed again. Sigh.
Arguably, now that the world is getting cleaner in javadoc-impl land, it would
be reasonable
to have internal taglets for FX stuff, even if we don't include the tags in the
standard set
supported by the DocTree API.
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.
I don't mind sprinkling the code in well-defined uses of well-defined
abstractions.
But yes, there could be more cleanup in this area.
Thanks for cleaning up the code along the way.
-- Jon
-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/
