On Mon, 31 Oct 2022 20:11:34 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:
>
> Add @SafeVarargs declarations
src/java.base/share/classes/java/lang/runtime/TemplateSupport.java line 74:
> 72:
> 73: /**
> 74: * Static final processor.
Suggestion:
* final processor.
src/java.base/share/classes/java/lang/runtime/TemplateSupport.java line 141:
> 139: MethodType processorGetterType = MethodType.methodType(ValidatingProcessor.class);
> 140: ValidatingProcessor<?, ?> processor =
> 141: (ValidatingProcessor<?, ?>)processorGetter.asType(processorGetterType).invokeExact();
Essentially the same as:
Suggestion:
ValidatingProcessor<?, ?> processor = (ValidatingProcessor<?, ?>)processorGetter.invoke();
src/java.base/share/classes/java/lang/runtime/TemplateSupport.java line 184:
> 182: MethodHandle mh = MethodHandles.insertArguments(DEFAULT_PROCESS_MH, 0, fragments, processor);
> 183: mh = mh.withVarargs(true);
> 184: mh = mh.asType(type);
I suggest doing:
Suggestion:
mh = mh.asCollector(Object[].class, type.parameterCount());
mh = mh.asType(type);
Instead, as it is more straightforward in terms of the code that gets called. (the impl of `withVarargs` + `asType` does the same thing in a more roundabout way).
src/java.base/share/classes/java/lang/template/ProcessorLinkage.java line 60:
> 58: * @throws NullPointerException if any of the arguments are null
> 59: */
> 60: MethodHandle linkage(List<String> fragments, MethodType type);
I suggest changing the protocol here to be able to take all bootstrap arguments into account, and return a `CallSite` instead. That will allow a `ProcessorLinkage` to take the lookup and name into account as well, and allows returning e.g. a `MutableCallSite` as well.
Maybe this can still be changed later as well though, since the interface is sealed.
src/java.base/share/classes/java/lang/template/StringTemplate.java line 103:
> 101: * // check or manipulate the fragments and/or values
> 102: * ...
> 103: * String result = StringTemplate.interpolate(fragments, values);;
Suggestion:
* String result = StringTemplate.interpolate(fragments, values);
src/java.base/share/classes/java/lang/template/StringTemplate.java line 117:
> 115: */
> 116: @PreviewFeature(feature=PreviewFeature.Feature.STRING_TEMPLATES)
> 117: public interface StringTemplate {
What is the reason for having this open to extension? Rather than just being a final class or sealed interface?
(I think it is so that implementations can provide a more efficient layout rather than using Lists?)
src/java.base/share/classes/java/lang/template/StringTemplate.java line 246:
> 244: Objects.requireNonNull(stringTemplate, "stringTemplate should not be null");
> 245: return Objects.hashCode(stringTemplate.fragments()) ^
> 246: Objects.hashCode(stringTemplate.values());
Could also (which is also `null` safe):
Suggestion:
return Objects.hash(stringTemplate.fragments(), stringTemplate.values());
src/java.base/share/classes/java/lang/template/ValidatingProcessor.java line 149:
> 147: @PreviewFeature(feature=PreviewFeature.Feature.STRING_TEMPLATES)
> 148: @FunctionalInterface
> 149: public interface ValidatingProcessor<R, E extends Throwable> {
I suggest moving the doc that gives an overview of the API here to the `package-info` file. The package-info file is a better place to provide an overview of the API I think. (that's what we do for `java.lang.foreign` as well: https://docs.oracle.com/en/java/javase/19/docs/api/java.base/java/lang/foreign/package-summary.html)
-------------
PR: https://git.openjdk.org/jdk/pull/10889