RFR: JDK-8237803 Reorganize impl of tool options

[Jonathan Gibbons]

Thanks for all the commentary/feedback.

-- Jon


On 01/24/2020 11:23 AM, Pavel Rappo wrote:
>   If I were to describe this change in a nutshell, it would be:
>
>     Map<ToolOption, Object> -> class ToolOptions

A slightly bigger, more accurate nutshell is

	{ Map<ToolOption, Object>, misc fields } -> class ToolOptions

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

Not entirely true.   Defaults are established, it just happens early on, 
in the field initialization.

Agreed that we lose the easy ability to determine if an option was 
specified.  We use that ability in javac, when checking for conflicts 
between incompatible options ... for example, all the 
platform-class-path related options and all the module-related options 
are allowed/disallowed depending on the source/release version. We don't 
need the ability here.  I note that an interim version of this work 
(before the review) left the enum in place, but then you have to pass in 
and use a reference to the enclosing ToolOptions object in all the 
"process" methods. If we needed to detect whether options were 
specified, we could certainly do something, such as maintaining a set of 
the ToolOption objects for options that have been found on the 
command-line. This could even be done in the option decoding loop in 
Start, and not in individual ToolOption objects.

> 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 least for now, the solution is to raise the bar, and fix comments on 
code that
are being worked on, and for reviewers to note stale comments.
> 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.

The intent is to make doclet options work the same way, i.e. encapsulate 
them.
I'll look for and remove the unnecessary SuppressWarnings.

> 4. Removing the `extends Helper` in Start looks a whole lot cleaner as it
> removes all the fields inherited from Helper.

Yes, that's a very old antipattern.  It almost leaked into this review, 
in that ToolOptions
was abstract, until I realized it would be better to use something like 
the ShowOptions
interface and use a local anon class.

-- Jon