On Mon, 14 Nov 2022 17:51:24 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: > > Wrong line separator Compiler changes look good to me. I've left some comments on API javadoc, as well as other minor issues. It would be great to address them now, but I understand if there's no time. src/java.base/share/classes/java/lang/runtime/TemplateRuntime.java line 115: > 113: * @throws Throwable if linkage fails > 114: */ > 115: public static CallSite stringTemplateBSM( In other classes in this package we never use `BSM` in the bootstrap name. Instead, we make some attempt to describe what the BSM does by picking some meaningful name (see SwitchBootstrap::typeSwitch). I think `newStringTemplate` (or `make`, `create`) would work here? src/java.base/share/classes/java/lang/template/ProcessorLinkage.java line 48: > 46: */ > 47: @PreviewFeature(feature=PreviewFeature.Feature.STRING_TEMPLATES) > 48: public sealed interface ProcessorLinkage permits FormatProcessor { Does this need to be exposed? FormatProcessor is the only permitted type, so javac could detect that (or have an internal list of supported optimized processors). src/java.base/share/classes/java/lang/template/StringProcessor.java line 31: > 29: > 30: /** > 31: * This interface simplifies declaration of Suggestion: * This interface simplifies the declaration of src/java.base/share/classes/java/lang/template/StringTemplate.java line 28: > 26: package java.lang.template; > 27: > 28: import java.lang.invoke.MethodHandle; Watch out for unused imports - I've seen them here and eslewhere src/java.base/share/classes/java/lang/template/StringTemplate.java line 38: > 36: > 37: /** > 38: * The Java compiler produces implementations of {@link StringTemplate} to I believe this descripton is now out of date - the compiler doesn't create implementations of StringTemplate, TemplateRuntime does - the compiler just request them. src/java.base/share/classes/java/lang/template/StringTemplate.java line 49: > 47: * The {@link StringTemplate#fragments()} method must return an immutable > 48: * {@code List<String>} consistent with the string template body. The list > 49: * contains the string of characters preceeding each of the embedded > expressions Suggestion: * contains the string of characters preceding each of the embedded expressions src/java.base/share/classes/java/lang/template/StringTemplate.java line 73: > 71: * {@code values} will be the equivalent of <code>List.of(x, y, x + > y)</code>. > 72: * <p> > 73: * {@link StringTemplate StringTemplates} are primarily used in conjuction Suggestion: * {@link StringTemplate StringTemplates} are primarily used in conjunction src/java.base/share/classes/java/lang/template/StringTemplate.java line 106: > 104: * } > 105: * > 106: * @implSpec An instance of {@link StringTemplate} is immutatble. Also, > the Suggestion: * @implSpec An instance of {@link StringTemplate} is immutable. Also, the src/java.base/share/classes/java/lang/template/StringTemplate.java line 120: > 118: /** > 119: * Returns an immutable list of string fragments consisting of the > string > 120: * of characters preceeding each of the embedded expressions plus the Suggestion: * of characters preceding each of the embedded expressions plus the src/java.base/share/classes/java/lang/template/StringTemplate.java line 138: > 136: * Returns an immutable list of embedded expression results. In the > example: > 137: * {@snippet : > 138: * StringTemplate st = RAW."\{x} + \{y} = \{x + y}"; Would it make sense to use the same student/teacher example as above? Or, perhaps, we should use x and y everywhere? src/java.base/share/classes/java/lang/template/StringTemplate.java line 150: > 148: > 149: /** > 150: * {@return the interpolation of the StringTemplate} A `@link` or `@code` is missing around `StringTemplate`. I think this method needs an example for what interpolation means. Also, in the class javadoc we use the term `string interpolation` which is more specific and I prefer. We should probably use that term everywhere in this class. src/java.base/share/classes/java/lang/template/StringTemplate.java line 173: > 171: * @param <E> Exception thrown type. > 172: * > 173: * @return constructed object of type R Missing `@code` or `@link` around `R` src/java.base/share/classes/java/lang/template/StringTemplate.java line 198: > 196: * @throws NullPointerException if stringTemplate is null > 197: */ > 198: public static String toString(StringTemplate stringTemplate) { `public` is redundant (here and elsewhere) src/java.base/share/classes/java/lang/template/StringTemplate.java line 208: > 206: > 207: /** > 208: * Returns a StringTemplate composed from a string. is `composed` the best term here? E.g. I'd prefer if this method actually told me what the properties of the returned template were - e.g. that it has one fragment (the string) and zero values. src/java.base/share/classes/java/lang/template/StringTemplate.java line 222: > 220: > 221: /** > 222: * Returns a StringTemplate composed from fragments and values. Suggestion: * Returns a StringTemplate with the given fragments and values. src/java.base/share/classes/java/lang/template/StringTemplate.java line 273: > 271: > 272: /** > 273: * Combine one or more {@link StringTemplate StringTemplates} to > produce a combined {@link StringTemplate}. Suggestion: * Combine one or more {@link StringTemplate StringTemplates} into a single {@link StringTemplate}. src/java.base/share/classes/java/lang/template/StringTemplate.java line 302: > 300: > 301: /** > 302: * No-op template processor. Used to highlight that non-processing > of the StringTemplate I wonder if there's a play to call this the "identity" processor. In the sense that it takes a template and just returns that unchanged. That would also help with calling it "raw" which creates confusion with some compiler messages. src/java.base/share/classes/java/lang/template/TemplateProcessor.java line 31: > 29: > 30: /** > 31: * This interface simplifies declaration of Suggestion: * This interface simplifies the declaration of src/java.base/share/classes/java/lang/template/TemplateProcessor.java line 44: > 42: * } > 43: * > 44: * @param <R> Processor's process result type. Some code block/link missing? src/java.base/share/classes/java/lang/template/ValidatingProcessor.java line 28: > 26: package java.lang.template; > 27: > 28: import java.util.Objects; Watch out for unused imports src/java.base/share/classes/java/lang/template/ValidatingProcessor.java line 41: > 39: * This interface describes the methods provided by a generalized string > template processor. The > 40: * primary method {@link ValidatingProcessor#process(StringTemplate)} is > used to validate > 41: * and compose a result using a {@link StringTemplate StringTemplate's} > fragments and values lists. `compose` as in `produce` ? src/java.base/share/classes/java/lang/template/ValidatingProcessor.java line 86: > 84: * Composing allows user control over how the result is assembled. Most > often, a > 85: * user will construct a new string from the template string, with > placeholders > 86: * replaced by stringified objects from the values list. stringified looks funny - but have no idea on how to replace it src/java.base/share/classes/java/lang/template/ValidatingProcessor.java line 126: > 124: * } > 125: * The {@link StringTemplate#interpolate()} method is available for > those processors > 126: * that just need to work with the interpolation; again, would be better to replace all bare occurrence of "interpolation" with "string interpolation" src/java.base/share/classes/java/util/FormatProcessor.java line 38: > 36: > 37: /** > 38: * This {@linkplain ValidatingProcessor template processor} constructs a > String `String` is missing some javadoc wrapping src/java.base/share/classes/java/util/FormatProcessor.java line 39: > 37: /** > 38: * This {@linkplain ValidatingProcessor template processor} constructs a > String > 39: * result using {@link Formatter}. Unlike {@link Formatter}, > FormatProcessor uses the value from And `FormatProcessor` too src/java.base/share/classes/java/util/FormatProcessor.java line 42: > 40: * the embedded expression that follows immediately after the > 41: * <a href="../util/Formatter.html#syntax">format specifier</a>. > 42: * StringTemplate expressions without a preceeding specifier, use "%s" by Suggestion: * StringTemplate expressions without a preceding specifier, use "%s" by src/java.base/share/classes/java/util/FormatProcessor.java line 51: > 49: * result is: <code>00010 + 00020 = 00030</code> > 50: * > 51: * @implNote When used in conjunction with a compiler generated {@link Is this comment still relevant? E.g. the `When used in conjunction with compiler generated...` part. Doesn't javac always emit a call to the optimized linkage entry if it sees that the processor used is a ProcessorLinkage? src/jdk.compiler/share/classes/com/sun/tools/javac/comp/TransLiterals.java line 125: > 123: } > 124: > 125: Type makeListType(Type elemType) { Unused src/jdk.compiler/share/classes/com/sun/tools/javac/comp/TransLiterals.java line 136: > 134: } > 135: > 136: JCVariableDecl makeField(JCClassDecl cls, long flags, Name name, > Type type, JCExpression init) { Unused src/jdk.compiler/share/classes/com/sun/tools/javac/comp/TransLiterals.java line 145: > 143: } > 144: > 145: MethodType makeMethodType(Type returnType, List<Type> argTypes) { Unused src/jdk.compiler/share/classes/com/sun/tools/javac/comp/TransLiterals.java line 149: > 147: } > 148: > 149: JCFieldAccess makeThisFieldSelect(Type owner, JCVariableDecl field) { Unused src/jdk.compiler/share/classes/com/sun/tools/javac/comp/TransLiterals.java line 156: > 154: } > 155: > 156: JCIdent makeParamIdent(List<JCVariableDecl> params, Name name) { Unused src/jdk.compiler/share/classes/com/sun/tools/javac/comp/TransLiterals.java line 171: > 169: } > 170: > 171: JCMethodInvocation makeApply(JCFieldAccess method, > List<JCExpression> args) { Unused src/jdk.compiler/share/classes/com/sun/tools/javac/comp/TransLiterals.java line 179: > 177: } > 178: > 179: JCFieldAccess makeFieldAccess(JCClassDecl owner, Name name) { Unused src/jdk.compiler/share/classes/com/sun/tools/javac/comp/TransLiterals.java line 223: > 221: JCStringTemplate tree; > 222: JCExpression processor; > 223: List<String> fragments; I still believe some of these fields could be final src/jdk.compiler/share/classes/com/sun/tools/javac/comp/TransLiterals.java line 318: > 316: staticArgValues, staticArgsTypes); > 317: } else { > 318: VarSymbol processorSym = > (VarSymbol)TreeInfo.symbol(processor); This seems unused src/jdk.compiler/share/classes/com/sun/tools/javac/resources/compiler.properties line 1301: > 1299: # 0: symbol > 1300: compiler.err.raw.template.processor.type=\ > 1301: raw template processor type: {0} This error message can be confusing now that we have a RAW processor type. I'd suggest maybe rephrasing to `template processor type cannot be a raw type` src/jdk.compiler/share/classes/com/sun/tools/javac/util/Names.java line 227: > 225: // templated string > 226: public final Name process; > 227: public final Name str; We have precedents for using capital variable names in this class, and we should probably do so here (for `str` and `raw`). ------------- Marked as reviewed by mcimadamore (Reviewer). PR: https://git.openjdk.org/jdk/pull/10889
