> On 30 Jan 2020, at 01:25, Jonathan Gibbons <[email protected]>
> wrote:
>
> Please review a moderately simple fix to a regression in the way that javadoc
> handles doclint options.
>
> The feature was introduced in JDK 8, and at that time, the doclet options
> were accumulated in a list.
>
> The regression was introduced when the standard doclet was rewritten in JDK
> 9, and perhaps because of the introduction of other new doclint options, the
> options were changed to accumulate in a map, with "last one wins" semantics
> for like-named options. This is the root cause of this issue.
Right. Arguments of an option are keyed by the instance, of the XOption type,
that handles this option. Like-named options are handled by the same instance of
the XOption type. Hence, when two sets of like-named options' arguments are
keyed, one of these sets is effectively overwritten.
This didn't necessarily mean we needed to do away with the map, we could use
`merge` or similar. But I agree with this change. We should keep it simple.
Nit. Could you please remove these (now obsolete) imports?
import java.util.LinkedHashMap;
import java.util.Map;
> The fix is to revert to accumulating options in a list, as originally
> intended. In addition, the detection of whether doclint should be enabled or
> not is improved, compared to JDK 8, such that doclint is only enabled if
> either specific groups or all groups are enabled. Previously, it would only
> be disabled if just -Xdoclint:none was specified.
Yes, current behavior seems be short-circuited by a stand-alone occurrence of
"none" (i.e. "-Xdoclint:none"). The proposed behavior always parses arguments
to completion, left to right. Is it expected behavior? Does javac do the same?
On a related note. I seem to understand how this code works. Still, why not
delegate options processing to DocLint itself? Is it because javadoc only
accepts a *subset* of DocLint options (e.g. no access qualifiers), or there
are other reasons?
> The penance for not writing a better unit test in JDK 8 is to write a better
> regression test now.
I think I dislike that the implementation package was added for the sake of
reusing the com.sun.tools.doclint.Messages.Group enum, but it is not that big
of a problem. It just makes the test "gray-box".
> One other test (added in JDK 9) was relying on the bug, and needed to be
> updated.
>
> In this changeset ...
>
> HtmlConfiguration.java ... the collection is changed to a list
>
> HtmlOptions ... the options are transformed into doclint options (as before)
> and accumulated in the list, but only if they pass the initial check. There
> is also some minor cleanup ... the option for -Xdocrootparent is moved out of
> the group of doclint options, and some minor warnings highlighted by the IDE
> were fixed.
Thanks for incremental improvements and miscellaneous cleanup!
180 doclint.init(t, doclintOpts.toArray(new String[0]), false);
What was the reason for changing this?
> WorkArounds ... the primary change here is better analysis of the doclint
> options to determine if doclint should be enabled. Comments are added, and
> the "TODO: fix this up correctly" is removed ;-)
>
> The new test uses a source file that will trigger different doclint
> diagnostics, depending on which are enabled. It then runs javadoc with
> different doclint options to verify that (just) the expected diagnostics are
> generated.
>
> -- Jon
>
>
> JBS: https://bugs.openjdk.java.net/browse/JDK-8236949
> Webrev: http://cr.openjdk.java.net/~jjg/8236949/webrev.00/
It would be nice if the original reporter could confirm this changeset solves
their issue (this email is CC'ed to them).
Looks good.
-Pavel
