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

Tue Jan 21 18:55:44 UTC 2020

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/

-- Jon

On 1/21/20 4:24 AM, Pavel Rappo wrote:
> Jon,
>
> I don't have a strong opinion on this refactoring. I will rely on your judgment
> and the fact that you know that code well.
I know it "well enough". While it would have been possible to do this 
cleanup
when we converted to the new doclet, a significant factor at that time 
was to
minimize cleanup like this, in order to make it easier to review the old
and new code side by side.  With some potential (minor) functional 
improvements
coming for the command-line options, this seems like a good opportunity
to clean up this code.

>
> My initial reaction was "Isn't it violating the Law of Demeter or something?"
> But on a closer look, I understood that it was merely a restructuring action.
> What you seem to be doing is trying to compartmentalize that code better.

Yes, the intent is to improve our acknowledgement of the Law of Demeter!

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.

Another potential cleanup, for another day, is to separate out the 
search index
tables, and any strongly related methods, from HtmlConfiguration into 
another
separate new abstraction.

>
> This refactoring preserves the collocation that exists between the code that
> parses the properties and the code that provides these properties for internal
> consumption.
>
> 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."

I changed to address an issue you make later on ... about a TreeSet
containing non-comparable objects. More on that later on.

>
> 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.

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.

>
> 3. I'm not sure what can be done about that TreeSet on HtmlOptions.java:458.
> Statically it operates on a non-comparable type, Doclet.Option.
Good catch; now fixed. This was the reason I fixed the wildcard you 
mentioned
earlier.  The intent is that the TreeSet was for BaseOptions.Option, which
*are* comparable. I did not go back and complete the signature edits.

>
> 4. What is the purpose of the BaseOptions.getOptions() method?
Another good catch. I thought it would be necessary, to allow the provision
of a covariant override in HtmlOptions, but that logic is flawed, since 
to make
use of the covariant override, you'd have to have an HtmlOptions object
in the first place.
>
> 5. HtmlConfiguration:170 feels like a debugging leftover. I'm not sure that
> this condition is even possible. Could it be an `assert` instead?

My bad; it was a debugging leftover. The condition can arise while in the
process of constructing the BaseOptions supertype of an HtmlOptions object.

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

Deleted

>
> 7. AbstractMemberWriter.java: 385, 603. Those could use the internal (alias)
> field `options`.

Yes, while editing the code, by itself, it did not seem worthwhile having
AbstractMemberWriter provide an alias, for just those two instances you
mention ... but it later became useful to provide the alias for the subtypes
of AbstractMemberWriter.

>
> 8. I noticed that BaseConfiguration have many blank lines. Was it done on purpose?
IDE leftovers, from moving content to the new classes. Excessive blank lines
removed.
>
> 9. Should BaseOptions use 2020 as the second copyright year?
I generally update copyrights after a review and before a push, to avoid
spurious changes showing up in the review. But since you have mentioned
it, I will fix the new files.
>
> 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"
>
> -Pavel

>
> 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/
>>
>>
>>