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 3 times, most recently from 1c0dd18 to 54a2f0c Compare November 30, 2025 08:16
@xuwei-k xuwei-k force-pushed the classfileVersionMap branch 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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport:nominated If we agree to backport this PR, replace this tag with "backport:accepted", otherwise delete it.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

use StringConcatFactory by default

5 participants