-
Notifications
You must be signed in to change notification settings - Fork 1.1k
update BackendUtils.classfileVersionMap
#23954
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
ba0b8c3 to
dd87778
Compare
| 14 -> asm.Opcodes.V14, | ||
| 15 -> asm.Opcodes.V15, | ||
| 16 -> asm.Opcodes.V16, | ||
| 17 -> asm.Opcodes.V17, |
There was a problem hiding this comment.
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`
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
1c0dd18 to
54a2f0c
Compare
54a2f0c to
eee5ebf
Compare
WojciechMazur
left a comment
There was a problem hiding this 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
scala3/compiler/src/dotty/tools/backend/jvm/BCodeBodyBuilder.scala
Lines 1306 to 1326 in 6bdf209
| // `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
There was a problem hiding this 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 |
There was a problem hiding this comment.
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 = |
There was a problem hiding this comment.
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)
delete old versions
https://www.scala-lang.org/news/next-scala-lts-jdk.html
StringConcatFactoryby default #24781