Re: RFR: 8329948: Remove string template feature [v4]
On Thu, 11 Apr 2024 09:12:06 GMT, Jan Lahoda wrote:
>> Maurizio Cimadamore has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Fix issues in digit classes
>
> test/langtools/jdk/jshell/CompletionSuggestionTest.java line 327:
>
>> 325: assertSignature("String.format(|",
>> 326: "String String.format(String, Object...)",
>> 327: "String String.format(java.util.Locale, String,
>> Object...)"
>
> Nit: this change appears to be unnecessary?
I'll revert - thanks
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18688#discussion_r1560775208
Re: RFR: 8329948: Remove string template feature [v4]
On Wed, 10 Apr 2024 10:59:25 GMT, Maurizio Cimadamore
wrote:
>> This PR removes support for the string template feature from the Java
>> compiler and the Java SE API, as discussed here:
>>
>> https://mail.openjdk.org/pipermail/amber-spec-experts/2024-April/004106.html
>
> Maurizio Cimadamore has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Fix issues in digit classes
javac and JShell changes look good to me (with a nit in JShell tests).
For consideration: using `{` will now produce the "illegal escape character"
error. Which is technically correct, but maybe we could add a special error,
saying that StringTemplates are removed for now? So that if someone will try to
compile source code with StringTemplates, they would now this was intentional.
Just for consideration.
test/langtools/jdk/jshell/CompletionSuggestionTest.java line 327:
> 325: assertSignature("String.format(|",
> 326: "String String.format(String, Object...)",
> 327: "String String.format(java.util.Locale, String,
> Object...)"
Nit: this change appears to be unnecessary?
-
Marked as reviewed by jlahoda (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/18688#pullrequestreview-1993673798
PR Review Comment: https://git.openjdk.org/jdk/pull/18688#discussion_r1560692978
Re: RFR: 8329948: Remove string template feature [v4]
On Wed, 10 Apr 2024 15:17:19 GMT, Jan Lahoda wrote: >> Good catch. Since this was tweaked to be a public API as part of the string >> template feature, I've reverted this code to what it was prior to string >> template. That is, now this is a private static final constant, with >> initializer set to 200 (no need to inhibit javac constant folding, since all >> uses are from this file). > > Just for completeness - note the field is not initialized in its > initializator, but using a static init below. Fields like this are not > inlined as constants by javac - that would require the value to be provided > in the field initializator. @lahodaj - I understand that - but I reverted to use the initializer, since the initializer was used before string templates (when the field was private). - PR Review Comment: https://git.openjdk.org/jdk/pull/18688#discussion_r1559676399
Re: RFR: 8329948: Remove string template feature [v4]
On Wed, 10 Apr 2024 10:44:13 GMT, Maurizio Cimadamore wrote: >> src/java.base/share/classes/java/lang/invoke/StringConcatFactory.java line >> 120: >> >>> 118: * @since 21 >>> 119: */ >>> 120: public static final int MAX_INDY_CONCAT_ARG_SLOTS; >> >> May this value change in the future? If yes, we might change this to a >> method to avoid incorrect eager inlining by the java compiler, or drop this >> with string templates. > > Good catch. Since this was tweaked to be a public API as part of the string > template feature, I've reverted this code to what it was prior to string > template. That is, now this is a private static final constant, with > initializer set to 200 (no need to inhibit javac constant folding, since all > uses are from this file). Just for completeness - note the field is not initialized in its initializator, but using a static init below. Fields like this are not inlined as constants by javac - that would require the value to be provided in the field initializator. - PR Review Comment: https://git.openjdk.org/jdk/pull/18688#discussion_r1559643723
Re: RFR: 8329948: Remove string template feature [v4]
> This PR removes support for the string template feature from the Java > compiler and the Java SE API, as discussed here: > > https://mail.openjdk.org/pipermail/amber-spec-experts/2024-April/004106.html Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision: Fix issues in digit classes - Changes: - all: https://git.openjdk.org/jdk/pull/18688/files - new: https://git.openjdk.org/jdk/pull/18688/files/a7396ce0..51e261bd Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18688=03 - incr: https://webrevs.openjdk.org/?repo=jdk=18688=02-03 Stats: 7 lines in 2 files changed: 0 ins; 6 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/18688.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18688/head:pull/18688 PR: https://git.openjdk.org/jdk/pull/18688
