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. 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. > On 24 Jan 2020, at 16:57, Jonathan Gibbons <[email protected]> > 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 <[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/ >>>
