On Mon, 31 Oct 2022 20:11:34 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: > > 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
