Skip to content

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Jan 23, 2018
Merged

Fix #3675: don't unnecessarily wrap arrays when eliminating java varargs #3869

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
46a9783
Fix #3675: don't unnecessarily wrap arrays when eliminating java varargs
Jan 18, 2018
7e18110
Add unit test and address review comments
Jan 19, 2018
09dbfc2
Pre-calculate the set of wrapXArray methods in Definitions
Jan 19, 2018
9c2f9c2
Use PerRun when calculating method symbols
Jan 19, 2018
5482916
Minor review fixes
Jan 22, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions compiler/src/dotty/tools/dotc/transform/ElimRepeated.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,8 @@ class ElimRepeated extends MiniPhase with InfoTransformer { thisPhase =>
private def seqToArray(tree: Tree, pt: Type)(implicit ctx: Context): Tree = tree match {
case SeqLiteral(elems, elemtpt) =>
JavaSeqLiteral(elems, elemtpt)
case app@Apply(fun, args) if isWrappedArray(app) =>
Copy link
Contributor

Choose a reason for hiding this comment

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

I would remove the comment below and add a comment here:

// rewrite a call to `wrapXArray(arr)` to `arr`

args.head // There's only one argument: the wrapped array
case _ =>
val elemType = tree.tpe.elemType
var elemClass = elemType.classSymbol
Expand All @@ -106,6 +108,14 @@ class ElimRepeated extends MiniPhase with InfoTransformer { thisPhase =>
// Because of phantomclasses, the Java array's type might not conform to the return type
}

/** Determines whether the given `tree` represents a wrapped array */
private def isWrappedArray(tree: Apply)(implicit ctx: Context): Boolean = {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@allanrenucci That's much nicer! Thanks! PTAL

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the name of the method and doc are confusing. This tests that the tree is a method call to Predef.wrappedXArray, not that the tree represents a wrapped array. I would prefer:

/** Is the tree a method call to Predef.wrapXArray */
def isWrapArrayMethodCall

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

val elemTpe = tree.tpe.elemType
val wrapMethodName = TreeGen.wrapArrayMethodName(elemTpe)
val wrapMethodSym = defn.ScalaPredefModuleRef.denot.requiredMethod(wrapMethodName)
tree.fun.symbol == wrapMethodSym
}

override def transformTypeApply(tree: TypeApply)(implicit ctx: Context): Tree =
transformTypeOfTree(tree)

Expand Down