On Fri, 28 Oct 2022 18:52:28 GMT, Rémi Forax <fo...@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
