On Mon, 10 Jul 2023 14:26:05 GMT, Pavel Rappo <pra...@openjdk.org> wrote:
>> Please review a medium-substantial cleanup of the internal `Taglet` world.
>>
>> The initial motivation was to move tag-specific functionality from
>> `TagletWriter[Impl]` to HTML-specific new subtypes of the individual
>> `Taglet` classes, so that taglets are now better represented by a
>> format-neutral base type and an HTML-specific subtype. The new subtypes are
>> collected in a new `formats.html.taglets` package, and for the most part,
>> they are accessed via their `Taglet` API.
>>
>> A secondary motivation is to clean up handling of inline tags. Not all
>> inline tags had corresponding taglets: most notably, `{@link}` was not
>> modeled by a taglet. Introducing `[Html]LinkTaglet` allowed more code to
>> move from `TagletWriterImpl` to `HtmlLinkTaglet` ... and `HtmlSeeTaglet` and
>> `HtmlSnippetTablet` now delegate to `HtmlLinkTaglet` to generate links.
>> Also, in `HtmlDocletWriter` a notable visitor, in `commentTags toContent`
>> had explicit `visit...` methods that effectively duplicated the
>> functionality of `defaultAction`, so those methods can be and have been
>> deleted.
>>
>> Taglets used to be stateless, even though they are generally created once
>> per doclet. (That was fixed, now, they _are_ created just once per doclet.)
>> They now contain the standard long-lived members, like `configuration`,
>> `utils`, `messages`, etc for the convenience of subtypes.
>> `TagletWriterImpl.Context` has always been effectively format-neutral and
>> has been moved up to `Taglet.Context`.
>>
>> It had been hoped to replace the `TagletWriter` parameter to
>> `Taglet::getInlineTagOutput` and `Taglet::getAllBlockTagOutput` with
>> `Taglet.Context` perhaps calling with a HTML-subtype instance. But there is
>> still enough useful functionality on `TagletWriter` that that is not
>> practical at this time.
>>
>> Taglets vary greatly in size, from small/trivial to large/complex. While it
>> might seem unnecessary to use top-level classes for the small case, it seems
>> better to go with a consistent uniform design rather than try and reduce any
>> perceived overhead, perhaps by selectively using nested classes, as is often
>> the case elsewhere in `jdk.javadoc` and `jdk.compiler`. Grouping the new
>> `HTML...Taglet` classes in a new `formats.html.taglets` package seems like a
>> good compromise.
>>
>> **Note**: this is just "cleanup" and refactoring. There is no intentional
>> change to any functionality, nor any added or removed. If code appears to
>> have been "deleted" it has either been moved elsewhere, or was effectively
>> unused anyway. No test...
>
> src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/LinkTaglet.java
> line 38:
>
>> 36: import jdk.javadoc.internal.doclets.toolkit.Content;
>> 37:
>> 38: public abstract class LinkTaglet extends BaseTaglet {
>
> `{@link}` has finally got its own taglet. However, I note that `{@code}` has
> lost its taglet.
>
> In the markup sense, `{@link}` is to `{@code}` as `{@linkplain}` is to
> `{@literal}`. If you agree with this, then I think we should leave `{@code}`,
> rather than `{@literal}`.
I agree, but may do that separately, or as part of subsequent cleanup
> src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/TagletManager.java
> line 190:
>
>> 188: this.utils = configuration.utils;
>> 189: this.tagletPath = options.tagletPath();
>> 190: // initStandardTaglets();
>
> A leftover?
oops yes.
> src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/TagletWriter.java
> line 107:
>
>> 105: * The context in which to generate the output for a series of
>> {@code DocTree} nodes.
>> 106: */
>> 107: public final Context context;
>
> I dislike it when a variable is accessible both directly and through the
> getter. Nonencapsulated fields like that is harder to debug and maintain.
> That said, I realise that it's somewhat of a pattern in jdk.javadoc. So, no
> need to change it at this time; I just make an observation.
Observation noted. For my part, I don't mind non-encapsulated fields if they
are final. I prefer that style to the "noisy" parens of getter methods:
var x = utils.something;
var x = utils().something;
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/14793#discussion_r1258476623
PR Review Comment: https://git.openjdk.org/jdk/pull/14793#discussion_r1258476968
PR Review Comment: https://git.openjdk.org/jdk/pull/14793#discussion_r1258479832
