The crux of the change is gathering options into a more monolithic and more
encapsulated design. If I were to describe this change in a nutshell, it would
be:
Map<ToolOption, Object> -> class ToolOptions
The change, as you described, does away with the enum. So ToolOption.values()
become ToolOptions.getSupportedOptions(). Equality checks (==) on ToolOption
constants turn into equality checks (equals()) on a handful of String constants
(DOCLET, DOCLET_PATH, DUMP_ON_ERROR, J, and LOCALE).
Commentary
==========
1. Map is a rich general-purpose interface. Still, ToolOptions could provide a
more apt interface for our needs. What we lose (at least in this changeset) is
the ability to control the behavior for nonexistent options (e.g.
getOrDefault/contains). Not sure if that was truly ever needed, though.
2. It's good to see fixes to Javadoc comments in the javadoc codebase.
If something is not continuously tested (built/compiled/run), it quickly gets
out of date. Some of the javadoc's javadoc comments are ridiculously inaccurate
or stale.
At this stage it is neither practical nor fair to ask ourselves to build the
documentation continuously fixing all the comments along the way. Still, we
should think about a solution to this. Meanwhile, thanks for doing this
manually, Jon!
3. The previous design, Map<ToolOption, Object>, imposed a lot of casting.
Though this could be solved using a ToolOption design similar to that of
java.net.SocketOption, the proposed design solves this problem another way.
Namely, read access to properties is performed through named methods with the
specific return types. Come to think of it, this should allow to get rid of
@SuppressWarning("unchecked") in places like:
- ElementsTable.java:409, 504
- Start.java:475
It's also good to see, naked (as opposed to Collections.emptyList())
Collections.EMPTY_LIST goes away.
4. Removing the `extends Helper` in Start looks a whole lot cleaner as it
removes all the fields inherited from Helper.
Looks good to me.
-Pavel
On 24 Jan 2020, at 02:30, Jonathan Gibbons <[email protected]> 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/
