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.
>>
>> 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.
>>
>> 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?
> 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.
Anyhow, I guess some of those overly verbose call sites could be treated with
"var".
>>
>> 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/
>>>
>
