On Tue, 21 Mar 2023 13:25:58 GMT, Maurizio Cimadamore <mcimadam...@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
