RFR: 8276124: Provide snippet support for properties files [v2]

[Pavel Rappo]

On Thu, 18 Nov 2021 19:02:33 GMT, Jonathan Gibbons <jjg at openjdk.org> 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