RFR: JDK-8237803 Reorganize impl of tool options

[Jonathan Gibbons]

On 1/24/20 9:11 AM, Pavel Rappo wrote:
> Hm... I mean that the ToolOption type uses javac types (e.g. com.sun.tools.javac.main.Option.OptionKind) in its methods signatures, and that it also throws javac exceptions (e.g. com.sun.tools.javac.main.Option.InvalidValueException).
>
> Please correct me if I'm wrong.
Not entirely wrong.
>
> It's as if javadoc borrows something that it doesn't have. In a perfect world this would suggest that there should be a common "module" dealing with the CLI options on which both tools would depend.

javadoc doesn't "borrow" it, it is almost compelled to use it, by virtue 
of delegating to the Option objects.

That being said, in this rewrite, there are a couple of methods 
(processCompilerOption) where we could catch/wrap/convert the javac 
exceptions and turn them into javadoc-only exceptions, but I'd have to 
think whether it's worth it.

As for a common module,

1. Early drafts of Project Jigsaw had much finer grain modules, but we 
backed off to the larger modules you see today. If we had smaller 
modules, my first split would be to separate the tool part of javadoc 
from the doclet part of javadoc.

2. There's ongoing chatter about a module for option decoding. We 
already have JOptSimple somewhere, and recently Brian G was chatting 
about another library that he found and liked.  My general opinion is 
that some common shared library would be good for new code, but would be 
hard to introduce into existing tools. For better or worse, javac has a 
mix of styles that has accreted over the years ...
     -multiwordname, --multi-word-name,  -Aname=value, -name:value, 
--name value, --name=value, -name/p:value, -name/a:value
     last-one-wins, values-accumulate, etc

There's also subtle differences:

  * javac keeps decoding options after --help, --version etc, and prints
    out info when all is done
  * javadoc handles --help, --version immediately and exits (I believe
    this is more like the launcher behavior.)

Changing all this (especially javac!) is more than I wanted to do in 
this changeset!

-- Jon

>
>> On 24 Jan 2020, at 16:57, Jonathan Gibbons <jonathan.gibbons at oracle.com> wrote:
>>
>> Re: com.sun.tools.javac.main.Option.*
>>
>> Well, it is deliberate and intentional that we fully delegate to javac those options that are handled by javac, like path-related options, --source, --release, --enable-preview etc. This reflects the fact that we totally rely on the javac front end to read source and class files.
>>
>> It was actually a big step forward in the JDK 9 rewrite that we properly delegate through the underlying Option objects, as compared to tunneling values into the compOpts object, as was done previously, which bypassed much of javac's checking.
>>
>> -- Jon
>>
>> On 1/24/20 8:51 AM, Pavel Rappo wrote:
>>> Hi Jon,
>>>
>>> I'm still working through the review but I have to say that degree to which com.sun.tools.javac.main.Option.* types have percolated to the javadoc internal interfaces is unsettling. I understand this is probably due to practical and historical reasons. Maybe we could address that later.
>>>
>>> -Pavel
>>>
>>>> On 24 Jan 2020, at 02:30, Jonathan Gibbons <jonathan.gibbons at oracle.com> wrote:
>>>>
>>>> Although the underlying problems are different, the general goal of this cleanup is similar in nature to that of the recent cleanup for doclet options.
>>>>
>>>> In this case, the effect is not as widespread ... just 6 source files affected, no tests ... but the changes to the main affected class are more substantial, although still primarily a refactoring and just moving code around, with no intentional change in functionality.
>>>>
>>>> To describe the changes, let me describe the world before this change:
>>>>
>>>> The ToolOption class followed the javac model for options and used an enum to represent the individual supported options. One problem of using an enum is that they are implicitly static, and so have never have any enclosing context. This means that when analyzing command-line arguments, the enum members need to be given an object providing the necessary context. In the case of ToolOption, this was a nested Helper class, which contained a mix of fields containing the values for some options, most notably those used in Start, and a map of objects for the values of other options, where the map was literally, Map<ToolOption,Object>. This led to "clunky" code to access the values in the map and to cast the result to the correct type for each value.
>>>>
>>>> In general, while there were some benefits to using the enum (such as being able to refer to some of the options by their member name), the cost outweighed the benefits.
>>>>
>>>> The primary change is to invert the nesting relationship between ToolOption and its Helper, and to rename and refactor the code accordingly.
>>>>
>>>> To summarize the changes,
>>>>
>>>> 1.    ToolOption.Helper becomes a new top-level class ToolOptions, which is the new primary abstraction for the accessing everything to do with tool options.
>>>>
>>>> 2.    ToolOption is changed from a top-level enum to a nested class in ToolOptions, with the members becoming a simple List<ToolOption>.
>>>>
>>>> 3.    All option values are represented as properly-typed encapsulated fields of ToolOptions. The fields are encapsulated, based on the feedback for the doclet options review.
>>>>
>>>> 4.    The direct use and passing around of the Map jdToolOpts is replaced by direct use of the new ToolOptions class.
>>>>
>>>> 5.    ToolOptions uses a new ShowHelper interface to separate out the functionality for handling options like --help and --verbose. Previously, Start implemented ToolOption.Help directly; now, it just uses a local anonymous class instead.
>>>>
>>>> 6.    ToolOption.java is renamed to ToolOptions.java, to retain history and to maximize the opportunity to compare the old and new versions.
>>>>
>>>> There are no significant changes to the high-level option handling in Start, which continues to do the double scan, to pick up selection options, like -doclet, -docletpath, -locale, before doing the main scan.  The handling of OptionException could also be simplified (separately), possibly allowing the ShowHelper class to be eliminated.
>>>>
>>>> One of the advantages of using the enum (in the old code) was that it allowed symbolic references to options handled in Start.preprocess.  These references are fixed up by defining string constants for the names of the handful of options in question, which is all that is needed.
>>>>
>>>> While the code is generally cleaner for allowing the ToolOption objects to be inner classes of ToolOptions, it does mean they can only exist in the context of a ToolOptions object. This has an impact on a little-used method on the DocumentationTask interface, to determine if an option name is supported. The body of the implementing method is moved into ToolOptions, which creates a temporary minimal ToolOptions object, sufficient to the needs of the isSupportedOption method.
>>>>
>>>> -- Jon
>>>>
>>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8237803
>>>> Webrev: http://cr.openjdk.java.net/~jjg/8237803/webrev/
>>>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/javadoc-dev/attachments/20200124/b6bba9ea/attachment.htm>