On Mon, 20 Mar 2023 20:31:46 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 two additional > commits since the last revision: > > - Tidy javadoc > - Rename StringTemplate classes src/java.base/share/classes/java/lang/StringTemplate.java line 449: > 447: * statically imported explicitly. > 448: */ > 449: static final SimpleProcessor<StringTemplate> RAW = st -> st; Should we omit the modifiers `static final`, which are implicit for all interface fields? src/java.base/share/classes/java/lang/StringTemplate.java line 455: > 453: * primary method {@link Processor#process(StringTemplate)} is used > to validate > 454: * and compose a result using a {@link StringTemplate > StringTemplate's} fragments and values lists. > 455: * Paragraph break intended here? src/java.base/share/classes/java/lang/StringTemplate.java line 550: > 548: * SimpleProcessor<JSONObject> jsonProcessor = st -> new > JSONObject(st.interpolate()); > 549: * } > 550: * @implNote The Java compiler automatically imports {@link > StringTemplate#STR} Does this note belong here? And does this come with a rule in the Java Language Specification, which can be linked? src/java.base/share/classes/java/lang/StringTemplate.java line 592: > 590: */ > 591: @PreviewFeature(feature=PreviewFeature.Feature.STRING_TEMPLATES) > 592: public sealed interface Linkage permits FormatProcessor { I fail to see any reason a user would call `linkage`; is there anything that's not covered by `TemplateRuntime::processStringTemplate`? And if users can't use this, this can just be relocated to one of the `jdk.internal` packages. src/java.base/share/classes/java/lang/StringTemplate.java line 679: > 677: @PreviewFeature(feature=PreviewFeature.Feature.STRING_TEMPLATES) > 678: @FunctionalInterface > 679: public interface StringProcessor extends SimpleProcessor<String> { Just curious, what's the rationale of a `SimpleProcessor` or `StringProcessor` as opposed to: static <T> TemplateProcessor<T, RuntimeException> simple(Function<StringTemplate, T> function) {...} static TemplateProcessor<String, RuntimeException> string(Function<StringTemplate, String> function) {...} which avoids the requirement of specifying the type of the template processor local variable; users can use `var` instead. src/java.base/share/classes/java/lang/invoke/StringConcatFactory.java line 1051: > 1049: * @param ptypes list of expression types > 1050: * > 1051: * @return {@link MethodHandle} Suggestion: * @return the {@link MethodHandle} for concatenation src/java.base/share/classes/java/lang/invoke/StringConcatFactory.java line 1169: > 1167: * have an extra {@link String} slot for the result from the > previous > 1168: * {@link MethodHandle}. > 1169: * {@link > java.lang.invoke.StringConcatFactory#makeConcatWithTemplate} Suggestion: * {@link #makeConcatWithTemplate} src/java.base/share/classes/java/lang/invoke/StringConcatFactory.java line 1246: > 1244: * This method creates a {@link MethodHandle} expecting one input, > the > 1245: * receiver of the supplied getters. This method uses > 1246: * {@link > java.lang.invoke.StringConcatFactory#makeConcatWithTemplateCluster} Suggestion: * {@link #makeConcatWithTemplateCluster} src/java.base/share/classes/java/lang/runtime/Carriers.java line 370: > 368: */ > 369: private static Map<MethodType, CarrierElements> > 370: methodTypeCache = ReferencedKeyMap.create(() -> new > ConcurrentHashMap<>()); Suggestion: methodTypeCache = ReferencedKeyMap.create(ConcurrentHashMap::new); src/java.base/share/classes/java/lang/runtime/Carriers.java line 421: > 419: */ > 420: protected CarrierObject(int primitiveCount, int objectCount) { > 421: this.primitives = createPrimitivesArray(primitiveCount ); Suggestion: this.primitives = createPrimitivesArray(primitiveCount); src/java.base/share/classes/java/lang/runtime/Carriers.java line 776: > 774: * @throws IllegalArgumentException if number of component slots > exceeds maximum > 775: */ > 776: static CarrierElements of(Class < ? >...ptypes) { Suggestion: static CarrierElements of(Class<?>... ptypes) { src/java.base/share/classes/java/lang/runtime/Carriers.java line 824: > 822: private CarrierElements() { > 823: throw new AssertionError("private constructor"); > 824: } Suggestion: src/java.base/share/classes/java/lang/runtime/ReferenceKey.java line 49: > 47: interface ReferenceKey<T> { > 48: /** > 49: * {@return the value of the unwrapped key.} Suggestion: * {@return the value of the unwrapped key} Inline return tag generates a period already. src/java.base/share/classes/java/lang/runtime/ReferencedKeyMap.java line 127: > 125: @SuppressWarnings("unchecked") > 126: static <K, V> ReferencedKeyMap<K, V> > 127: create(boolean isSoft, Supplier<Map<?, ?>> supplier) { What prevents this to take `Supplier<Map<ReferencedKey<K>, V>>`? Same for below. src/java.base/share/classes/java/lang/runtime/StringTemplateImpl.java line 40: > 38: * <p> > 39: * Values are stored by subclassing {@link Carriers.CarrierObject}. This > allows specializations > 40: * and sharing of value shapes without creating a new class for each > shape. Just curious, what specific benefits does subclassing offer over having `CarrierObject` as a field? I don't see how a new class is ever created for each shape in that scenario, as shapes are only used to construct the 2 MHs in the factory. src/java.base/share/classes/java/lang/runtime/StringTemplateImpl.java line 65: > 63: * {@link java.lang.invoke.CallSite CallSite}. > 64: */ > 65: private final MethodHandle valuesMH; I don't think `java.lang.runtime` is a package where final fields are [trusted](https://github.com/openjdk/jdk/blob/91f407d6fe285c44bcc25c1acdf5dc0c43be0172/src/hotspot/share/ci/ciField.cpp#L226), so these method handles might have a performance overhead. However, records appear to be optimized and eligible for inlining. src/java.base/share/classes/java/lang/runtime/StringTemplateImplFactory.java line 88: > 86: * > 87: * @param fragments string template fragments > 88: * @param type method type Suggestion: * @param type method type accepting values' types and returning a StringTemplate src/java.base/share/classes/java/lang/runtime/StringTemplateImplFactory.java line 169: > 167: * @return StringTemplate composed from fragments and values > 168: */ > 169: static StringTemplate newStringTemplate(List<String> fragments, > Object[] values) { I recommend renaming this one and the above methods (taking Object[] for values) `newTrustedStringTemplate` to indicate these two trust the passed values array. src/java.base/share/classes/java/lang/runtime/TemplateRuntime.java line 204: > 202: Object[] values > 203: ) throws Throwable { > 204: List<Object> asList = Collections.unmodifiableList(new > ArrayList<>(Arrays.asList(values))); Suggestion: List<Object> asList = List.of(values); For defensive copy. Don't think processors are harmed by the null-hostile behavior of these list, for many template implementations already use null-hostile lists. src/java.base/share/classes/java/lang/runtime/TemplateRuntime.java line 233: > 231: */ > 232: private static StringTemplate fromArrays(String[] fragments, > Object[] values) { > 233: return StringTemplateImplFactory.newStringTemplate(fragments, > values); Should perform defensive copying for the input arrays, specifically `values` (fragments is passed to `List.of`, which is already safe) In addition, I recommend renaming methods that don't perform defensive copies to like `ofTrusted`, so we can better avoid such problems. src/java.base/share/classes/java/util/Digits.java line 68: > 66: */ > 67: final class DecimalDigits implements Digits { > 68: private static final short[] DIGITS; Can add `@Stable` to speed up array element access. Same for below. src/java.base/share/classes/java/util/FormatProcessor.java line 157: > 155: Objects.requireNonNull(stringTemplate); > 156: String format = stringTemplateFormat(stringTemplate.fragments()); > 157: Object[] values = stringTemplate.values().toArray(new Object[0]); Suggestion: Object[] values = stringTemplate.values().toArray(); ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/10889#discussion_r1145500281 PR Review Comment: https://git.openjdk.org/jdk/pull/10889#discussion_r1145500568 PR Review Comment: https://git.openjdk.org/jdk/pull/10889#discussion_r1145503416 PR Review Comment: https://git.openjdk.org/jdk/pull/10889#discussion_r1145582334 PR Review Comment: https://git.openjdk.org/jdk/pull/10889#discussion_r1145509343 PR Review Comment: https://git.openjdk.org/jdk/pull/10889#discussion_r1145511968 PR Review Comment: https://git.openjdk.org/jdk/pull/10889#discussion_r1145520158 PR Review Comment: https://git.openjdk.org/jdk/pull/10889#discussion_r1145521984 PR Review Comment: https://git.openjdk.org/jdk/pull/10889#discussion_r1145532679 PR Review Comment: https://git.openjdk.org/jdk/pull/10889#discussion_r1145528633 PR Review Comment: https://git.openjdk.org/jdk/pull/10889#discussion_r1145531061 PR Review Comment: https://git.openjdk.org/jdk/pull/10889#discussion_r1145531547 PR Review Comment: https://git.openjdk.org/jdk/pull/10889#discussion_r1145533037 PR Review Comment: https://git.openjdk.org/jdk/pull/10889#discussion_r1145534263 PR Review Comment: https://git.openjdk.org/jdk/pull/10889#discussion_r1145566980 PR Review Comment: https://git.openjdk.org/jdk/pull/10889#discussion_r1145555878 PR Review Comment: https://git.openjdk.org/jdk/pull/10889#discussion_r1145567708 PR Review Comment: https://git.openjdk.org/jdk/pull/10889#discussion_r1145576079 PR Review Comment: https://git.openjdk.org/jdk/pull/10889#discussion_r1145577290 PR Review Comment: https://git.openjdk.org/jdk/pull/10889#discussion_r1145570455 PR Review Comment: https://git.openjdk.org/jdk/pull/10889#discussion_r1145578159 PR Review Comment: https://git.openjdk.org/jdk/pull/10889#discussion_r1145579950
