Re: RFR: [15] JDK-8237492 Reorganize impl of doclet options
Hi Jon,
Sorry for the late arrival, I did not do a deep dive, however this caught my
eye.
I realize you must have used the IDE to refactor, in any case, in the lines you
have changed there
are these old constructs:
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/AbstractOverviewIndexWriter.java
+if (doctitle.length() > 0) {
These can be replaced with doctitle.isEmpty().
On Jan 22, 2020, at 11:29 AM, Jonathan Gibbons
mailto:[email protected]>> wrote:
Pavel,
Thanks for your additional feedback. There's a couple of actionable items, most
notably
with respect to StandardDoclet.
As for the rest, I agree there's a whole bunch more stuff that we *could* do,
but I would
prefer to get the work so far staged into the repo. As the ancient Roman's used
to say,
javadoc was not cleaned up in a single changeset.
Very true indeed!. :)
Kumar
-- Jon
On 01/22/2020 08:00 AM, Pavel Rappo wrote:
On 21 Jan 2020, at 18:55, Jonathan Gibbons
mailto:[email protected]>> wrote:
Pavel,
All great feedback, and all points addressed, as described in the details
inline below.
New webrev, addresses all your comments, adds a couple of class-level doc
comments
to the two new classes, and fixes a couple of inconsequential spelling errors.
Otherwise,
no changes in all the other affected files.
https://nam04.safelinks.protection.outlook.com/?url=http:%2F%2Fcr.openjdk.java.net%2F~jjg%2F8237492%2Fwebrev.01%2F&data=02%7C01%7Ckusrinivasan%40vmware.com%7Ccb446ac59a204a6741f608d79f71c5fe%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637153183321632928&sdata=Tio9bquIDiwfRh4R8bmfrMqxShy8JP5KlW%2FdmBeQv60%3D&reserved=0
Thanks for patiently addressing my comments. I see this code review as an
opportunity to get familiar with the javadoc code base.
The ongoing task is to draw lines around parts of the hodge-podge that is
{Base,Html}Configuration, and to pull out those parts into separate, better
defined abstractions.
A noble intent.
1. You have reintroduced a forgotten bounded wildcard to
public Set getSupportedOptions()
Good. Compatibility-wise this should be benign. Hopefully, no one tries to put
anything into that set, which should be assumed unmodifiable anyway.
It's only an internal API, and we control the implementations. As they say,
"No public API was affected in the making of this changeset."
StandardDoclet is a public SPI. Doclets may extend that class, but it's the
"container" that calls the `getSupportedOptions` method. A corner case where the
client calls `getSupportedOptions` would be an implementation of Doclet that
delegates calls to an internal instance of StandardDoclet:
public class MyDoclet implements Doclet {
private final StandardDoclet standardDoclet = new StandardDoclet();
// ...
@Override
public Set getSupportedOptions() {
Set supportedOptions = standardDoclet.getSupportedOptions();
supportedOptions.add(new MyOption()); // additional option
// ...
return supportedOptions;
}
private static class MyOption implements Doclet.Option {
// ...
}
// ...
}
Agreed, this is a somewhat contrived example made for the sake of the argument.
You're right; I'd missed that this was a change to StandardDoclet, which is a
public API.
This will need to be sorted out, separately.
2. You consistently used camelCase naming for fields that represent options.
This effectively "unlinks" them from their command-line names, which is not bad.
Fewer possibilities to mess this during (automated) future refactorings if you
ask me.
The option names are often horrible and do not provide a really good
precedent.
Agreed.
It's tempting to an an informational source-only annotation that identifies
the options that affect each field, but without any checking, such annotations
would be little better than comments ... which is why I added comments
to identify the options for each value.
This could be addressed another way. Instead of having two separate
abstractions,
options classes and option fields, we could use a single abstraction, types.
We could use some sort of a container [1]. The downside might be having more
types. A somewhat related design can be seen in java.net.SocketOption API [2].
That latter API tackles the need for more types by relying on option names, yet
still benefits from the type-safety.
That could allow for more collocation of the code related to command-line
options.
I think this is more than I want to consider for this round of cleanup.
6. AbstractMemberWriter's fields `printedSummaryHeader` and `nodepr` seem not to
be used. Can those be deleted?
Deleted
The `BaseOptions.docFileDestDirName` field doesn't seem to be accessed from
anywhere. Should it be deleted?
Yes, will do.
While we are in this area, consider hyphenating "command line" where it is a
compound adjective rather than a noun (possibly, n
Re: RFR: [15] JDK-8237492 Reorganize impl of doclet options
Kumar,
Thanks for the feedback; I'll incorporate this change. (I'm currently
waiting on a minor CSR approval).
-- Jon
On 01/23/2020 09:47 AM, Kumar Srinivasan wrote:
Hi Jon,
Sorry for the late arrival, I did not do a deep dive, however this
caught my eye.
I realize you must have used the IDE to refactor, in any case, in the
lines you have changed there
are these old constructs:
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/AbstractOverviewIndexWriter.java
+ if (doctitle.length() > 0) {
These can be replaced with doctitle.isEmpty().
On Jan 22, 2020, at 11:29 AM, Jonathan Gibbons
mailto:[email protected]>> wrote:
Pavel,
Thanks for your additional feedback. There's a couple of actionable
items, most notably
with respect to StandardDoclet.
As for the rest, I agree there's a whole bunch more stuff that we
*could* do, but I would
prefer to get the work so far staged into the repo. As the ancient
Roman's used to say,
javadoc was not cleaned up in a single changeset.
Very true indeed!. :)
Kumar
-- Jon
On 01/22/2020 08:00 AM, Pavel Rappo wrote:
On 21 Jan 2020, at 18:55, Jonathan Gibbons
mailto:[email protected]>>
wrote:
Pavel,
All great feedback, and all points addressed, as described in the
details inline below.
New webrev, addresses all your comments, adds a couple of
class-level doc comments
to the two new classes, and fixes a couple of inconsequential
spelling errors. Otherwise,
no changes in all the other affected files.
https://nam04.safelinks.protection.outlook.com/?url=http:%2F%2Fcr.openjdk.java.net%2F~jjg%2F8237492%2Fwebrev.01%2F&data=02%7C01%7Ckusrinivasan%40vmware.com%7Ccb446ac59a204a6741f608d79f71c5fe%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637153183321632928&sdata=Tio9bquIDiwfRh4R8bmfrMqxShy8JP5KlW%2FdmBeQv60%3D&reserved=0
Thanks for patiently addressing my comments. I see this code review
as an
opportunity to get familiar with the javadoc code base.
The ongoing task is to draw lines around parts of the hodge-podge
that is
{Base,Html}Configuration, and to pull out those parts into
separate, better
defined abstractions.
A noble intent.
1. You have reintroduced a forgotten bounded wildcard to
public Set getSupportedOptions()
Good. Compatibility-wise this should be benign. Hopefully, no one
tries to put
anything into that set, which should be assumed unmodifiable anyway.
It's only an internal API, and we control the implementations. As
they say,
"No public API was affected in the making of this changeset."
StandardDoclet is a public SPI. Doclets may extend that class, but
it's the
"container" that calls the `getSupportedOptions` method. A corner
case where the
client calls `getSupportedOptions` would be an implementation of
Doclet that
delegates calls to an internal instance of StandardDoclet:
public class MyDoclet implements Doclet {
private final StandardDoclet standardDoclet = new
StandardDoclet();
// ...
@Override
public Set getSupportedOptions() {
Set supportedOptions =
standardDoclet.getSupportedOptions();
supportedOptions.add(new MyOption()); // additional option
// ...
return supportedOptions;
}
private static class MyOption implements Doclet.Option {
// ...
}
// ...
}
Agreed, this is a somewhat contrived example made for the sake of
the argument.
You're right; I'd missed that this was a change to StandardDoclet,
which is a public API.
This will need to be sorted out, separately.
2. You consistently used camelCase naming for fields that
represent options.
This effectively "unlinks" them from their command-line names,
which is not bad.
Fewer possibilities to mess this during (automated) future
refactorings if you
ask me.
The option names are often horrible and do not provide a really good
precedent.
Agreed.
It's tempting to an an informational source-only annotation that
identifies
the options that affect each field, but without any checking, such
annotations
would be little better than comments ... which is why I added comments
to identify the options for each value.
This could be addressed another way. Instead of having two separate
abstractions,
options classes and option fields, we could use a single
abstraction, types.
We could use some sort of a container [1]. The downside might be
having more
types. A somewhat related design can be seen in
java.net.SocketOption API [2].
That latter API tackles the need for more types by relying on option
names, yet
still benefits from th
RFR: JDK-8237803 Reorganize impl of tool options
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. 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. 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/
