On Fri, 28 Oct 2022 17:57:30 GMT, Jim Laskey <jlas...@openjdk.org> wrote:

>> Enhance the Java programming language with string templates, which are 
>> similar to string literals but contain embedded expressions. A string 
>> template is interpreted at run time by replacing each expression with the 
>> result of evaluating that expression, possibly after further validation and 
>> transformation. This is a [preview language feature and 
>> API](http://openjdk.java.net/jeps/12).
>
> 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

src/java.base/share/classes/java/lang/invoke/StringConcatFactory.java line 1060:

> 1058:             throws StringConcatException
> 1059:     {
> 1060:         Objects.requireNonNull(fragments, "fragments is null");

I think you need to do some defensive copy here

  ptypes = List.copyOf(pTypes);

to avoid the types and fragments to be changed at the same time they are 
checked.

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

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.

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

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

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

Reply via email to