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
