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

[Rémi Forax]

On Fri, 28 Oct 2022 17:57:30 GMT, Jim Laskey <jlaskey at 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/template/StringTemplate.java line 307:

> 305:             Objects.requireNonNull(fragment, "fragments elements must be non-null");
> 306:         }
> 307:         fragments = Collections.unmodifiableList(new ArrayList<>(fragments));

I think using `List.copyOf()` is more efficient that `Collections.unmodifiableList(new ArrayList<>(...))` because there is no copy if the list is already created with List.of().

src/java.base/share/classes/java/lang/template/StringTemplate.java line 323:

> 321:      * @throws NullPointerException fragments or values is null or if any of the fragments is null
> 322:      */
> 323:     public static String interpolate(List<String> fragments, List<Object> values) {

This method also exists has a static method, having both is a bad idea because it makes StringTemplate::interpolate a compile error, the compiler has no way to know that it's the same implementation.

src/java.base/share/classes/java/lang/template/StringTemplate.java line 354:

> 352:      * @implNote The result of interpolation is not interned.
> 353:      */
> 354:     public static final StringProcessor STR = st -> st.interpolate();

Should be `StringTemplate::interpolate`.

src/java.base/share/classes/java/lang/template/TemplateProcessor.java line 38:

> 36:  * that do not throw checked exceptions. For example:
> 37:  * {@snippet :
> 38:  * TemplateProcessor<String> processor = st -> {

This is a good example of why having both way to describe a template processor of string, TemplateProcessor<String and `StringPorcessor` is a bad idea.

src/java.base/share/classes/java/lang/template/TemplateRuntime.java line 45:

> 43:  */
> 44: @PreviewFeature(feature=PreviewFeature.Feature.STRING_TEMPLATES)
> 45: public final class TemplateRuntime {

Why this class is public ? and it should be called `TemplateProcessors` linke all other classes in Java that store a bunch of static methods (Collections, Collectors, etc)

src/java.base/share/classes/java/lang/template/TemplateRuntime.java line 65:

> 63:      * @throws Throwable            if linkage fails
> 64:      */
> 65:     public static CallSite stringTemplateBSM(

I wonder if this method should be moved to a class named `TemplateProcesorFactory` inside `java.lang.runtime`? Like the all the bootstrap methods recently added.

src/java.base/share/classes/java/lang/template/TemplateRuntime.java line 79:

> 77:         MethodType processorGetterType = MethodType.methodType(ValidatingProcessor.class);
> 78:         ValidatingProcessor<?, ? extends Throwable> processor =
> 79:                 (ValidatingProcessor<?, ? extends Throwable>)processorGetter.asType(processorGetterType).invokeExact();

`ValidatingProcessor<?, ?>` should be enough ? No ?
Not using a "? extends Throwable" here make the type unchecked.

src/java.base/share/classes/java/lang/template/TemplateRuntime.java line 88:

> 86:      * Manages the boostrapping of {@link ProcessorLinkage} callsites.
> 87:      */
> 88:     private static class TemplateBootstrap {

This class should be `final`

src/java.base/share/classes/java/lang/template/TemplateRuntime.java line 117:

> 115:          * Static final processor.
> 116:          */
> 117:         private final ValidatingProcessor<?, ? extends Throwable> processor;

Use `ValidatingProcessor<?, ?>` here

src/java.base/share/classes/java/lang/template/TemplateRuntime.java line 145:

> 143:         private TemplateBootstrap(MethodHandles.Lookup lookup, String name, MethodType type,
> 144:                                   List<String> fragments,
> 145:                                   ValidatingProcessor<?, ? extends Throwable> processor) {

Use ValidatingProcessor<?, ?> here

src/java.base/share/classes/java/lang/template/TemplateRuntime.java line 211:

> 209:     @SuppressWarnings("unchecked")
> 210:     public static <E> List<E> toList(E... elements) {
> 211:         return Collections.unmodifiableList(Arrays.asList(elements));

This is List.of(), please use List.of() instead

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

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