-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Fix #3675: don't unnecessarily wrap arrays when eliminating java varargs #3869
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
Changes from 4 commits
46a9783
7e18110
09dbfc2
9c2f9c2
5482916
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,6 +9,7 @@ import scala.collection.{ mutable, immutable } | |
import PartialFunction._ | ||
import collection.mutable | ||
import util.common.alwaysZero | ||
import dotty.tools.dotc.transform.TreeGen | ||
|
||
object Definitions { | ||
|
||
|
@@ -338,6 +339,12 @@ class Definitions { | |
def Predef_classOf(implicit ctx: Context) = Predef_classOfR.symbol | ||
lazy val Predef_undefinedR = ScalaPredefModule.requiredMethodRef("???") | ||
def Predef_undefined(implicit ctx: Context) = Predef_undefinedR.symbol | ||
// The set of all wrap{X, Ref}Array methods, where X is a value type | ||
lazy val Predef_wrapArrayR: collection.Set[TermRef] = { | ||
val methodNames = ScalaValueTypes.map(TreeGen.wrapArrayMethodName) + nme.wrapRefArray | ||
methodNames.map(ScalaPredefModule.requiredMethodRef) | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would inline this in val Predef_wrapArray = new PerRun[collection.Set[Symbol]] { implicit ctx =>
val methodNames = ScalaValueTypes.map(TreeGen.wrapArrayMethodName) + nme.wrapRefArray
methodNames.map(name => ScalaPredefModule.requiredMethodRef(name).toSymbol)
} |
||
val Predef_wrapArray = new PerRun[collection.Set[Symbol]](implicit ctx => Predef_wrapArrayR.map(_.symbol)) | ||
|
||
lazy val ScalaRuntimeModuleRef = ctx.requiredModuleRef("scala.runtime.ScalaRunTime") | ||
def ScalaRuntimeModule(implicit ctx: Context) = ScalaRuntimeModuleRef.symbol | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -187,4 +187,32 @@ class TestBCode extends DottyBytecodeTest { | |
diffInstructions(instructions1, instructions2)) | ||
} | ||
} | ||
|
||
/** Verifies that arrays are not unnecessarily wrapped when passed to Java varargs methods */ | ||
@Test def dontWrapArraysInJavaVarargs = { | ||
val source = | ||
""" | ||
|import java.nio.file._ | ||
|class Test { | ||
| def test(xs: Array[String]) = { | ||
| val p4 = Paths.get("Hello", xs: _*) | ||
| } | ||
|} | ||
""".stripMargin | ||
|
||
checkBCode(source) { dir => | ||
val moduleIn = dir.lookupName("Test.class", directory = false) | ||
val moduleNode = loadClassNode(moduleIn.input) | ||
val method = getMethod(moduleNode, "test") | ||
|
||
val arrayWrapped = instructionsFromMethod(method).exists { instr: Instruction => | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can do: val arrayWrapped = instructionsFromMethod(method).exists {
case inv: Invoke => inv.name.contains("wrapRefArray")
case _ => false
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
instr match { | ||
case inv: Invoke => inv.name.contains("wrapRefArray") | ||
case _ => false | ||
} | ||
} | ||
|
||
assert(!arrayWrapped, "Arrays should not be wrapped when passed to a Java varargs method\n") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I verified that this fails if I comment out the changes to ElimRepeated. |
||
} | ||
} | ||
} |
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.
@smarter pre-calculated the set of methods here, as suggested
PTAL