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