<i18n dev> RFR: JDK-8285932 Implementation of JEP 430 String Templates (Preview) [v46]

Wed Mar 22 14:43:43 UTC 2023

On Tue, 21 Mar 2023 13:25:58 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

>> Jim Laskey has updated the pull request incrementally with two additional commits since the last revision:
>> 
>>  - Tidy javadoc
>>  - Rename StringTemplate classes
>
> src/java.base/share/classes/java/lang/StringTemplate.java line 28:
> 
>> 26: package java.lang;
>> 27: 
>> 28: import java.lang.Object;
> 
> You might want to do another pass to check for unused imports

Changing

> src/java.base/share/classes/java/lang/StringTemplate.java line 44:
> 
>> 42:  * {@link StringTemplate} is the run-time representation of a string template or
>> 43:  * text block template in a template expression.
>> 44:  *
> 
> Extra newline?

Changing

> src/java.base/share/classes/java/lang/StringTemplate.java line 56:
> 
>> 54:  * {@link StringTemplate} is primarily used in conjunction with a template processor
>> 55:  * to produce a string or other meaningful value. Evaluation of a template expression
>> 56:  * first produces an instance of {@link StringTemplate}, representing the template
> 
> The "template of the template expression" - is this nomenclature official (e.g. backed by any text in the JLS?). I must admit I find it a tad jarring.

"representing the right hand side of the template expression"

> src/java.base/share/classes/java/lang/StringTemplate.java line 324:
> 
>> 322:      * assert Objects.equals(sc, "abcxyz");
>> 323:      * }
>> 324:      * the last character {@code "c"} from the first string is juxtaposed with the first
> 
> I was playing with this example in jshell:
> 
> jshell> var st1 = RAW."{1}"
> st1 ==> StringTemplate{ fragments = [ "", "" ], values = [1] }
> 
> jshell> var st2 = RAW."{2}"
> st2 ==> StringTemplate{ fragments = [ "", "" ], values = [2] }
> 
> jshell> StringTemplate.combine(st1, st2);
> $7 ==> StringTemplate{ fragments = [ "", "", "" ], values = [1, 2] }
> 
> 
> The result is obviously well-formed - but I'm not sure I can derive what the method does just by reading the javadoc. The javadoc says:
> 
> Fragment lists from the StringTemplates are combined end to end with the last fragment from each StringTemplate concatenated with the first fragment of the next
> 
> I think I see what you want to say - (e.g. the end fragment of string template `N` is concatenated with the starting fragment of string template `N + 1`).

okay

> src/java.base/share/classes/java/lang/StringTemplate.java line 431:
> 
>> 429:      * Java compilation unit.<p>The result of interpolation is not interned.
>> 430:      */
>> 431:     static final StringProcessor STR = StringTemplate::interpolate;
> 
> `static final` is redundant here (we are in an interface)

Did not know that.

> src/java.base/share/classes/java/lang/StringTemplate.java line 454:
> 
>> 452:      * This interface describes the methods provided by a generalized string template processor. The
>> 453:      * primary method {@link Processor#process(StringTemplate)} is used to validate
>> 454:      * and compose a result using a {@link StringTemplate StringTemplate's} fragments and values lists.
> 
> nit: `{@link StringTemplate StringTemplate's}` will probably not render as you'd expect (as it will all be preformat, including the `'s`).

works for me

> src/java.base/share/classes/java/lang/runtime/ReferenceKey.java line 47:
> 
>> 45:  */
>> 46: @PreviewFeature(feature=PreviewFeature.Feature.STRING_TEMPLATES)
>> 47: interface ReferenceKey<T> {
> 
> This (and other) class are package-private. Do we still need @PreviewFeature for stuff like this, since it's not meant to be used publicly?

Informative incase some one wants to use elsewhere in the JDK. My plan is to move this class to java.util at some point.

> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/TransLiterals.java line 226:
> 
>> 224:                         List.nil(), expressions);
>> 225:                 valuesArray.type = new ArrayType(syms.objectType, syms.arrayClass);
>> 226:                 return bsmCall(names.process, names.newLargeStringTemplate, syms.stringTemplateType,
> 
> nit: the indy name is irrelevant here - that said, `process` is a tad confusing, since it's also the name of a StringTemplate method.

Tracks well in javah

> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/TransLiterals.java line 252:
> 
>> 250:                 staticArgsTypes = staticArgsTypes.append(syms.stringType);
>> 251:             }
>> 252:             return bsmCall(names.process, names.processStringTemplate, tree.type,
> 
> What happens if we have too many fragments here? (e.g. > 200). That case seems handled in the RAW case.

Criteria in isLinkageProcessor() prevents this from being called.

        boolean isLinkageProcessor() {
            return processor != null &&
                   !useValuesList &&  <=====
                   types.isSubtype(processor.type, syms.linkageType) &&
                   processor.type.isFinal() &&
                   TreeInfo.symbol(processor) instanceof VarSymbol varSymbol &&
                   varSymbol.isStatic() &&
                   varSymbol.isFinal();
        }

> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/TransLiterals.java line 281:
> 
>> 279:             make.at(tree.pos);
>> 280: 
>> 281:             if (processor == null || isNamedProcessor(names.RAW)) {
> 
> is `processor == null` still a thing?

I deliberately left this to track free standing templates. Will be phased out in next preview/final.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/10889#discussion_r1144898843
PR Review Comment: https://git.openjdk.org/jdk/pull/10889#discussion_r1144899315
PR Review Comment: https://git.openjdk.org/jdk/pull/10889#discussion_r1144902350
PR Review Comment: https://git.openjdk.org/jdk/pull/10889#discussion_r1144907969
PR Review Comment: https://git.openjdk.org/jdk/pull/10889#discussion_r1144911867
PR Review Comment: https://git.openjdk.org/jdk/pull/10889#discussion_r1144916768
PR Review Comment: https://git.openjdk.org/jdk/pull/10889#discussion_r1144915315
PR Review Comment: https://git.openjdk.org/jdk/pull/10889#discussion_r1144920121
PR Review Comment: https://git.openjdk.org/jdk/pull/10889#discussion_r1144924240
PR Review Comment: https://git.openjdk.org/jdk/pull/10889#discussion_r1144918724