Skip to content

Conversation

@xuwei-k
Copy link
Contributor

@xuwei-k xuwei-k commented Sep 18, 2025

@xuwei-k xuwei-k force-pushed the classfileVersionMap branch from ba0b8c3 to dd87778 Compare September 18, 2025 13:03
14 -> asm.Opcodes.V14,
15 -> asm.Opcodes.V15,
16 -> asm.Opcodes.V16,
17 -> asm.Opcodes.V17,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a comment here:

//java 8~16 support has been dropped`

@xuwei-k

This comment was marked as outdated.

@xuwei-k

This comment was marked as outdated.

@xuwei-k xuwei-k closed this Sep 18, 2025
@xuwei-k xuwei-k deleted the classfileVersionMap branch September 21, 2025 11:41
@xuwei-k xuwei-k restored the classfileVersionMap branch November 28, 2025 12:06
@xuwei-k xuwei-k reopened this Nov 28, 2025
@xuwei-k xuwei-k force-pushed the classfileVersionMap branch 4 times, most recently from 54a2f0c to eee5ebf Compare December 1, 2025 00:24
@xuwei-k xuwei-k marked this pull request as ready for review December 1, 2025 02:06
@Gedochao Gedochao added the backport:nominated If we agree to backport this PR, replace this tag with "backport:accepted", otherwise delete it. label Dec 22, 2025
@Gedochao Gedochao added this to the 3.8.0 milestone Dec 22, 2025
@Gedochao Gedochao requested a review from noti0na1 December 22, 2025 07:50
Copy link
Contributor

@WojciechMazur WojciechMazur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you for these changes.

Potentially we can also remove the now deadcode logic for String Concat used in JDK8

// `StringConcatFactory` only got added in JDK 9, so use `StringBuilder` for lower
if (backendUtils.classfileVersion < asm.Opcodes.V9) {
// Estimate capacity needed for the string builder
val approxBuilderSize = concatArguments.view.map {
case Literal(Constant(s: String)) => s.length
case Literal(c @ Constant(_)) if c.isNonUnitAnyVal => String.valueOf(c).length
case _ => 0
}.sum
bc.genNewStringBuilder(approxBuilderSize)
stack.push(jlStringBuilderRef) // during the genLoad below, there is a reference to the StringBuilder on the stack
for (elem <- concatArguments) {
val elemType = tpeTK(elem)
genLoad(elem, elemType)
bc.genStringBuilderAppend(elemType)
}
stack.pop()
bc.genStringBuilderEnd
} else {

However it can also be done in the follow up. I leave the decision up to you, we'd most likely merge it tomorrow and backport to 3.8.0-RC5

Copy link
Member

@tanishiking tanishiking left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One nitpick, but otherwise LGTM from myside. (but we can merge it without fixing if we're in rush)

"append(D)Ljava/lang/StringBuilder;",
"append(Ljava/lang/StringBuffer;)Ljava/lang/StringBuilder;",
"append(Ljava/lang/CharSequence;)Ljava/lang/StringBuilder;",
"append(Ljava/lang/Object;)Ljava/lang/StringBuilder;", // test that we're not using the [C overload
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because Java9+ uses https://openjdk.org/jeps/280 👍

class StringInterpolatorOptTest extends DottyBytecodeTest {
import ASMConverters._

private def convertInvokeDynamicArray(i: Instruction): Instruction =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this is converting arrays to lists for structural equality of InvokeDynamic, but bsmArgs shouldn't have Array...
It turned out this is the consequence of a long-time bug in AsmConverters 😄 The following diff should fix without convertInvokeDynamicArray.

diff --git a/compiler/test/dotty/tools/backend/jvm/AsmConverters.scala b/compiler/test/dotty/tools/backend/jvm/AsmConverters.scala
index c751937bd9..dfa7ed25ab 100644
--- a/compiler/test/dotty/tools/backend/jvm/AsmConverters.scala
+++ b/compiler/test/dotty/tools/backend/jvm/AsmConverters.scala
@@ -148,7 +148,7 @@ object ASMConverters {
 
     private def convertBsmArgs(a: Array[Object]): List[Object] = a.iterator.map({
       case h: asm.Handle => convertMethodHandle(h)
-      case _ => a // can be: Class, method Type, primitive constant
+      case x => x // can be: Class, method Type, primitive constant
     }).toList
 
     private def convertMethodHandle(h: asm.Handle): MethodHandle = MethodHandle(h.getTag, h.getOwner, h.getName, h.getDesc, h.isInterface)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@xuwei-k xuwei-k force-pushed the classfileVersionMap branch from eee5ebf to 2b92b26 Compare December 26, 2025 04:07
@xuwei-k xuwei-k force-pushed the classfileVersionMap branch from 2b92b26 to 042bd3f Compare December 26, 2025 04:09
Copy link
Member

@tanishiking tanishiking left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@tanishiking tanishiking merged commit 82acfa7 into scala:main Dec 26, 2025
95 of 96 checks passed
@xuwei-k xuwei-k deleted the classfileVersionMap branch December 26, 2025 07:10
@xuwei-k
Copy link
Contributor Author

xuwei-k commented Dec 26, 2025

Potentially we can also remove the now deadcode logic for String Concat used in JDK8

WojciechMazur pushed a commit that referenced this pull request Dec 26, 2025
@WojciechMazur WojciechMazur added backport:accepted This PR needs to be backported, once it's been backported replace this tag by "backport:done" and removed backport:nominated If we agree to backport this PR, replace this tag with "backport:accepted", otherwise delete it. labels Dec 30, 2025
WojciechMazur added a commit that referenced this pull request Dec 31, 2025
Backports #23954 to the 3.8.0-RC5.

PR submitted by the release tooling.
[skip ci]
@WojciechMazur WojciechMazur added backport:done This PR was successfully backported. and removed backport:accepted This PR needs to be backported, once it's been backported replace this tag by "backport:done" labels Dec 31, 2025
WojciechMazur added a commit that referenced this pull request Jan 13, 2026
Backports #23954 to the 3.8.1-RC1.

PR submitted by the release tooling.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport:done This PR was successfully backported.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

use StringConcatFactory by default

5 participants