On Mon, 15 Nov 2021 20:34:20 GMT, Pavel Rappo <pra...@openjdk.org> wrote:
> The initial integration (JDK-8266666) of JEP 413 did not support properties
> files. This commit rights that wrong.
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.
I'd also recommend not using an `enum` for `Language` to leave open the
possibility of easily/dynamically adding support for additional languages.
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.
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?
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?
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
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
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)
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?
-------------
PR: https://git.openjdk.java.net/jdk/pull/6397
