On 06/20/2016 04:18 PM, Kumar Srinivasan wrote:
Hello,
Please review the changes to fix:
https://bugs.openjdk.java.net/browse/JDK-8159305
The webrev is here:
http://cr.openjdk.java.net/~ksrini/8159305/webrev.00/
The spec-diff is here for reference:
http://cr.openjdk.java.net/~ksrini/8159305/spec-diff/overview-summary.html
Thanks
Kumar
DocletEnvironment: general changes will require a CCC.
DocletEnvironment:61
"annotations" is wrong. You mean "annotation types". An "annotation"
is the *use* of an annotation type.
line 101, don't like the "style" of plural type names (like
"ModuleElements") in plain text. Type names should be in {@code} font.
Consider using the underlying English words. (module elements, package
elements, etc). If in doubt, follow whatever style Joe uses in the 269 API.
linbe 142. terse
doclet/package-info:63 plural type names again
Should use code form for any occurrence of an option name
60/61 the name is "included" but the <em> term is "specified". Not good
to be inconsistent.
89 uses both, I'm not usre if the two are the same or different.
AbstractExecutableMemberWriter, 118, who is "we"? Could improve
phrasing, but I accept it's internal API.
AnnotationTypeRequiredMemberImpl, 108, OK, itr's an overloaded method,
but I simply note that in this override, the parameter is ignored.
ConfigurationImpl 422
replace DocPath.empty.resolve with plain old "new DocPath"
AbstractDoclet 119
Uugh
Configuration 358
Bad grammar/punctuation. I'm surprised by the ordering of the decls
for modules and modulePackages. I would have thought that modules is the
dominant declaration here, and the modulePackages is derived from
modules; instead, the comment makes it seem like it's the other way
around. And, according to the code in initModules, the description is
wrong anyway.
Configuration 392, initModules, similar to preceding comment. I think
you have the role of modules and modulePackages backwards. Create
"modules" first, add in the specifiedModules, then add in the packages.
Configuration 420, why the guard, are you worried about calling this
multiple times?
TypeElementCatalog 103, redundant TypeELementCatalog.this (looks like NB
error)
Utils is a bit of a mess, taken in conjunction with Configuration.
There's a no-reason split between stuff in Utils and stuff in Configuration.
DocEnv
funky imports order
DocEnv.ModulePackage ... an interface? really? why not just a class
containing two public final strings?
JavadocTool ... IncludesTable looks big enough to be split into its own
top level class, and *maybe* should start becoming the basis of a new
abstraction for all "included" stuff on the command, i.e. superseding
code from Configuration and Utils.
JavadocTool.IncludesTable.ModulePackage. Over-engineered.
RootDocImpl:
more lack of abstraction with regard to stuff specified on the command line.
ToolOption
add M as an alias for MODULE
New long form options (SHOW_MODULES, etc) use space or = as a separator,
not :
ToolOption.AccessHelper looks misguided. Either the functionality here
should be merged into Helper, or it should possibly be renamed and
become a member of Helper that supersedes/replaces the showAccess map
in Helper.
-- Jon
