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

[Jim Laskey]

On Fri, 28 Oct 2022 18:52:28 GMT, Rémi Forax <forax at openjdk.org> wrote:

>> Jim Laskey has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Update TemplateRuntime::combine
>
> src/java.base/share/classes/java/lang/invoke/StringConcatFactory.java line 1042:
> 
>> 1040:      * The number of fragments must be one more that the number of ptypes.
>> 1041:      * The total number of slots used by the ptypes must be less than or equal
>> 1042:      * to MAX_INDY_CONCAT_ARG_SLOTS.
> 
> see my comment about making MAX_INDY_CONCAT_ARG_SLOTS public

Disagree.

> src/java.base/share/classes/java/lang/invoke/StringConcatFactory.java line 1177:
> 
>> 1175:      */
>> 1176:     @PreviewFeature(feature=PreviewFeature.Feature.STRING_TEMPLATES)
>> 1177:     public static List<MethodHandle> makeConcatWithTemplateCluster(
> 
> I think instead of having two public methods and the user having to choose between them, it's better to have the implementations private and on public methods that calls the correct implementation if maxSlots > MAX_INDY_CONCAT_ARG_SLOTS or not

Use cases are very different.  The first one produces a `MethodHandle` that has multiple inputs, The second one produces a `MethodHandle` that can only have one input.

> src/java.base/share/classes/java/lang/template/ProcessorLinkage.java line 51:
> 
>> 49:     /**
>> 50:      * Construct a {@link MethodHandle} that constructs a result based on the
>> 51:      * bootstrap method information.
> 
> This comment is quite obscure if you have no idea how it works.
> And the information that the returned method handle has the type of the MethodType passed as parameter is missing.

Deliberate obscure for preview. Once we sort out the functional space you have been looking for, this will likely go away. ProcessorFactory and ProcessorBuilder are a couple of the possibilities.

> src/java.base/share/classes/java/lang/template/SimpleStringTemplate.java line 38:
> 
>> 36: record SimpleStringTemplate(List<String> fragments,
>> 37:                              List<Object> values
>> 38: ) implements StringTemplate {}
> 
> A compact constructor doing defensive copies is missing

The defensive copies are done by the callers.

> src/java.base/share/classes/java/lang/template/StringProcessor.java line 45:
> 
>> 43: @PreviewFeature(feature=PreviewFeature.Feature.STRING_TEMPLATES)
>> 44: @FunctionalInterface
>> 45: public interface StringProcessor extends TemplateProcessor<String> {}
> 
> The name should be `StringTemplateProcessor`.
> And i'm not sure it's useful to have a specialized version for String, TemplateProcessor<String> is not an issue given that most of the time people will implement it, so writing `implements StringTemplateProcessor` instead of `implements TemplateProcessor<String>` does not seem to offer enough bangs for bucks.
> 
> see TemplateProcessor

Wrong use case. Think `StringProcessor upper = st -> st.interpolate().toUpperCase();`

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

PR: https://git.openjdk.org/jdk/pull/10889