RFR: [15] JDK-8237492 Reorganize impl of doclet options

[Pavel Rappo]

> On 21 Jan 2020, at 18:55, Jonathan Gibbons <jonathan.gibbons at oracle.com> wrote:
> 
> Pavel,
> 
> All great feedback, and all points addressed, as described in the details inline below.
> 
> New webrev, addresses all your comments, adds a couple of class-level doc comments
> to the two new classes, and fixes a couple of inconsequential spelling errors. Otherwise,
> no changes in all the other affected files.
> 
> http://cr.openjdk.java.net/~jjg/8237492/webrev.01/

Thanks for patiently addressing my comments. I see this code review as an
opportunity to get familiar with the javadoc code base.

> <snip>
> 
> The ongoing task is to draw lines around parts of the hodge-podge that is
> {Base,Html}Configuration, and to pull out those parts into separate, better
> defined abstractions.

A noble intent.

> <snip>
> 
>> 1. You have reintroduced a forgotten bounded wildcard to
>> 
>>     public Set<? extends Doclet.Option> getSupportedOptions()
>> 
>> Good. Compatibility-wise this should be benign. Hopefully, no one tries to put
>> anything into that set, which should be assumed unmodifiable anyway.
> 
> It's only an internal API, and we control the implementations. As they say,
> "No public API was affected in the making of this changeset."

StandardDoclet is a public SPI. Doclets may extend that class, but it's the
"container" that calls the `getSupportedOptions` method. A corner case where the
client calls `getSupportedOptions` would be an implementation of Doclet that
delegates calls to an internal instance of StandardDoclet:

    public class MyDoclet implements Doclet {

        private final StandardDoclet standardDoclet = new StandardDoclet();

        // ...

        @Override
        public Set<? extends Option> getSupportedOptions() {
            Set<Option> supportedOptions = standardDoclet.getSupportedOptions();
            supportedOptions.add(new MyOption()); // additional option
            // ...
            return supportedOptions;
        }

        private static class MyOption implements Doclet.Option {
            // ...
        }
        // ...
    }

Agreed, this is a somewhat contrived example made for the sake of the argument.

>> 2. You consistently used camelCase naming for fields that represent options.
>> This effectively "unlinks" them from their command-line names, which is not bad.
>> Fewer possibilities to mess this during (automated) future refactorings if you
>> ask me.
> 
> The option names are often horrible and do not provide a really good
> precedent.

Agreed.

> It's tempting to an an informational source-only annotation that identifies
> the options that affect each field, but without any checking, such annotations
> would be little better than comments ... which is why I added comments
> to identify the options for each value.

This could be addressed another way. Instead of having two separate abstractions,
options classes and option fields, we could use a single abstraction, types.

We could use some sort of a container [1]. The downside might be having more
types. A somewhat related design can be seen in java.net.SocketOption API [2].
That latter API tackles the need for more types by relying on option names, yet
still benefits from the type-safety.

That could allow for more collocation of the code related to command-line options.

> <snip>
> 
>> 6. AbstractMemberWriter's fields `printedSummaryHeader` and `nodepr` seem not to
>> be used. Can those be deleted?
> 
> Deleted

The `BaseOptions.docFileDestDirName` field doesn't seem to be accessed from
anywhere. Should it be deleted?

> <snip>
> 
>> While we are in this area, consider hyphenating "command line" where it is a
>> compound adjective rather than a noun (possibly, not an exhaustive list):
>> 
>>   * HtmlConfiguration: 54, 56
>>   * HtmlOptions: 68, 74, 87, 125, 132, 138, 144, 162
>>   * BaseConfiguration: 396, 693
>>   * BaseOptions: 178
>>   * IllegalOptionValue: 32
>>   * Main: 49, 58, 70, 83
>>   * javadoc.properties: 94
> I fixed all instances found by searching for "command line opt"

Thanks!


The below is not a criticism of the refactoring, but rather a commentary on
the current state of affairs.

I don't like that command-line options are represented by public fields that can
be freely accessed from any part of the code. What I find especially unsettling
is that some of the fields of the BaseOptions and HtmlOptions are written from
the outside of those classes. These fields include:

    BaseOptions.docEncoding,
    BaseOptions.javafx,
    HtmlOptions.charset

This field is written *only* from the outside of HtmlOptions:

    HtmlOptions.createOverview

All that points out that Configuration and Options are coupled more tightly than
we may think.

-Pavel

--------------------------------------------------------------------------------
[1] Effective Java, Second Edition by Joshua Bloch,
        Item 29: Consider typesafe heterogeneous containers
[2] See java.net.SocketOption, java.net.StandardSocketOptions,
java.net.Socket.supportedOptions, java.net.Socket.getOption, java.net.Socket.setOption

>> P.S. Thanks for being super careful and not only updating the javadoc comments
>> but also the commented out code in SourceToHTMLConverter!
>> 
>>> On 18 Jan 2020, at 01:51, Jonathan Gibbons <jonathan.gibbons at oracle.com> wrote:
>>> 
>>> Please review a code-cleanup to reorganize the code to handle doclet options.
>>> 
>>> Fundamentally, the change is to move the methods and fields to handle option processing from BaseConfiguration and HtmlConfiguration out into new abstractions BaseOptions and HtmlOptions.  As such, the dominant changes are to these 4 files.
>>> 
>>> The impact on the rest of the doclet code is just to change where to access the values for options: instead of accessing the values directly from the *Configuration classes, the values are now obtained from the corresponding *Option classes, which are in turn accessed from the *Configuration classes. The reference to the Options objects are typically cached when there are a large number of usages in the code. In a  number of cases, the cache is in a supertype, which reduces the visible churn.
>>> 
>>> I've taken this opportunity to rename the fields for the values of options into the standard camelCase convention. And, I've done some basic work to clean up comments, although more could be done (later).
>>> 
>>> Fixing a bunch of spurious warnings uncoverable a real warning, that the code was creating a sorted set of incomparable Option objects. This changeset also fixes that issue, which mostly just shows up in the signatures for internal collections of option objects.
>>> 
>>> There is no change in functionality here. All the tests pass without change, except for one that was tunneling into an impl class, and which needed to be updated.
>>> 
>>> There's probably a similar cleanup coming to the code to handle tool options.
>>> 
>>> -- Jon
>>> 
>>> 
>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8237492
>>> Webrev: http://cr.openjdk.java.net/~jjg/8237492/webrev/