On Thu, 18 Nov 2021 19:02:33 GMT, Jonathan Gibbons <[email protected]> wrote:
>> Pavel Rappo has updated the pull request with a new target base due to a
>> merge or a rebase. The incremental webrev excludes the unrelated changes
>> brought in by the merge/rebase.
>
> src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/SnippetTaglet.java
> line 67:
>
>> 65: public class SnippetTaglet extends BaseTaglet {
>> 66:
>> 67: public enum Language {
>
> The idea of a `Language` object is good, but using an `enum` precludes easy
> extensibility.
We'll figure out the extensibility when the need comes. The language of a
snippet defines both the comment marker and the comment type. For now, an
alternative to that enum would be a boolean flag, which is less readable.
> src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/snippet/Parser.java
> line 87:
>
>> 85: int payloadEnd();
>> 86: int markupStart();
>> 87: }
>
> I'm surprised/disappointed that this may be necessary, although I accept
> there is a notable difference between "whole-line comments" and "end-of-line
> comments". Is it possible to reduce `CoarseParser` down to a
> language-specific regular expression with named groups for the payload and
> markup, such that the regular expression could be a property of the
> `Language` object?
I'm going to answer both of your comments, this and that one above:
> I think you can simplify the code by reducing `CoarseParser` down to a
> language-specific regular expression with "standard" named groups for the
> payload and markup.
Regular expressions with named groups were the initial design. However, I
changed it to `CoarseParser` halfway through the implementation, when saw how
bulky the named regex were becoming. As you also note, "end-of-line comments"
and "comment lines" differ. I couldn't quickly come up with a regex that
accounts for both of them.
> src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/snippet/Parser.java
> line 93:
>
>> 91:
>> 92: @Override
>> 93: public boolean parse(String line) {return
>> m.reset(line).matches();}
>
> Given that matchers are lightweight objects, what is the benefit of using a
> pre-allocated matcher and `reset` as compared to a temporary `Matcher`
> object? Is it just style, or is there something more?
Mostly style, I think.
> src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/snippet/Parser.java
> line 99:
>
>> 97:
>> 98: @Override
>> 99: public int markupStart() {return m.start(3);}
>
> Suggest using named groups
I'll answer this below.
> test/langtools/jdk/javadoc/doclet/testSnippetTag/TestLangProperties.java line
> 44:
>
>> 42: * @build javadoc.tester.* toolbox.ToolBox toolbox.ModuleBuilder
>> builder.ClassBuilder
>> 43: * @run main TestLangProperties
>> 44: */
>
> move before imports
Done.
> test/langtools/jdk/javadoc/doclet/testSnippetTag/TestLangProperties.java line
> 47:
>
>> 45: public class TestLangProperties extends SnippetTester {
>> 46:
>> 47: private final ToolBox tb = new ToolBox();
>
> Suggest moving to SnippetTester (suggested in dependent PR)
Done.
> test/langtools/jdk/javadoc/doclet/testSnippetTag/TestLangProperties.java line
> 49:
>
>> 47: private final ToolBox tb = new ToolBox();
>> 48:
>> 49: private TestLangProperties() {}
>
> Just curious: why force this?
You mean, why make that constructor private? No reason. This file started as a
copy-paste. Fixed.
-------------
PR: https://git.openjdk.java.net/jdk/pull/6397
