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
<[email protected] <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
<[email protected] <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
<https://nam04.safelinks.protection.outlook.com/?url=http:%2F%2Fcr.openjdk.java.net%2F%7Ejjg%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.
<snip>
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.
<snip>
1. You have reintroduced a forgotten bounded wildcard to
public Set<? extends Doclet.Option> 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<? extends Option> getSupportedOptions() {
Set<Option> 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.
<snip>
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.
<snip>
While we are in this area, consider hyphenating "command line"
where it is a
compound adjective rather than a noun (possibly, not an exhaustive
list):
* HtmlConfiguration: 54, 56
* HtmlOptions: 68, 74, 87, 125, 132, 138, 144, 162
* BaseConfiguration: 396, 693
* BaseOptions: 178
* IllegalOptionValue: 32
* Main: 49, 58, 70, 83
* javadoc.properties: 94
I fixed all instances found by searching for "command line opt"
Thanks!
The below is not a criticism of the refactoring, but rather a
commentary on
the current state of affairs.
I don't like that command-line options are represented by public
fields that can
be freely accessed from any part of the code. What I find especially
unsettling
is that some of the fields of the BaseOptions and HtmlOptions are
written from
the outside of those classes. These fields include:
BaseOptions.docEncoding,
BaseOptions.javafx,
HtmlOptions.charset
This field is written *only* from the outside of HtmlOptions:
HtmlOptions.createOverview
I agree that the mutable public fields are not great, and that we
might want to use
an IDE to encapsulate the fields. The 3 fields that are modified
from inside and out of
the class have default values being set. If/when we encapsulate the
fields, it may be
possible to handle the fault in the access method.
The same probably goes for HtmlOptions.createOverview.
All that points out that Configuration and Options are coupled more
tightly than
we may think.
No, it just means we have not quite got the boundary in quite the
right place yet.
But, they are clearly related since part of the configuration is the
options, but there
is more code in the configuration classes that uses the options to
determine more
configuration values.
-Pavel
--------------------------------------------------------------------------------
[1] Effective Java, Second Edition by Joshua Bloch,
Item 29: Consider typesafe heterogeneous containers
[2] See java.net.SocketOption, java.net.StandardSocketOptions,
java.net.Socket.supportedOptions, java.net.Socket.getOption,
java.net.Socket.setOption
P.S. Thanks for being super careful and not only updating the
javadoc comments
but also the commented out code in SourceToHTMLConverter!
On 18 Jan 2020, at 01:51, Jonathan Gibbons
<[email protected]
<mailto:[email protected]>> wrote:
Please review a code-cleanup to reorganize the code to handle
doclet options.
Fundamentally, the change is to move the methods and fields to
handle option processing from BaseConfiguration and
HtmlConfiguration out into new abstractions BaseOptions and
HtmlOptions. As such, the dominant changes are to these 4 files.
The impact on the rest of the doclet code is just to change where
to access the values for options: instead of accessing the values
directly from the *Configuration classes, the values are now
obtained from the corresponding *Option classes, which are in
turn accessed from the *Configuration classes. The reference to
the Options objects are typically cached when there are a large
number of usages in the code. In a number of cases, the cache is
in a supertype, which reduces the visible churn.
I've taken this opportunity to rename the fields for the values
of options into the standard camelCase convention. And, I've done
some basic work to clean up comments, although more could be done
(later).
Fixing a bunch of spurious warnings uncoverable a real warning,
that the code was creating a sorted set of incomparable Option
objects. This changeset also fixes that issue, which mostly just
shows up in the signatures for internal collections of option
objects.
There is no change in functionality here. All the tests pass
without change, except for one that was tunneling into an impl
class, and which needed to be updated.
There's probably a similar cleanup coming to the code to handle
tool options.
-- Jon
JBS:
https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugs.openjdk.java.net%2Fbrowse%2FJDK-8237492&data=02%7C01%7Ckusrinivasan%40vmware.com%7Ccb446ac59a204a6741f608d79f71c5fe%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637153183321632928&sdata=w2xJGSbuItjHTHfg95KhpWbtsBOSgR02JoLoxwHGPvM%3D&reserved=0
Webrev:
https://nam04.safelinks.protection.outlook.com/?url=http:%2F%2Fcr.openjdk.java.net%2F~jjg%2F8237492%2Fwebrev%2F&data=02%7C01%7Ckusrinivasan%40vmware.com%7Ccb446ac59a204a6741f608d79f71c5fe%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637153183321632928&sdata=bcgajooeHWoA5vUjrjEsmImYBOcdf21Y3o49jg7tdPc%3D&reserved=0
<https://nam04.safelinks.protection.outlook.com/?url=http:%2F%2Fcr.openjdk.java.net%2F%7Ejjg%2F8237492%2Fwebrev%2F&data=02%7C01%7Ckusrinivasan%40vmware.com%7Ccb446ac59a204a6741f608d79f71c5fe%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637153183321632928&sdata=bcgajooeHWoA5vUjrjEsmImYBOcdf21Y3o49jg7tdPc%3D&reserved=0>
