Fix #2916: Reorder parameters #2931
Conversation
| val newOrder = (liftedDefs zip indices).sortBy(_._2).unzip._1 | ||
| liftedDefs = ListBuffer.empty | ||
| liftedDefs ++= newOrder | ||
| liftedDefs = (liftedDefs zip indices).sortBy(_._2).unzip._1.to |
| case nil => -1 | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Why not simply:
firstDiff(xs, ys.dropWhile(_ == EmptyTree), n)
?
| val (liftable, rest) = typedArgs splitAt (typedArgs.length - eqSuffixLength) | ||
|
|
||
| // Mapping of index of each `liftable` into original args ordering |
There was a problem hiding this comment.
Lots of state here; better to move this out to a separate method and describe what it does in a doc comment.
| nextDefaultParamIndex += 1 | ||
| } | ||
| } | ||
|
|
||
| typedArgs = liftArgs(liftedDefs, methType, liftable) ++ rest |
There was a problem hiding this comment.
Why does it not work if we lift args before reordering?
nicolasstucki
force-pushed
the
fix-i2916
branch
from
cf55e6a to
ee8a4a8
Compare
nicolasstucki
force-pushed
the
fix-i2916
branch
from
ee8a4a8 to
733673f
Compare
|
|
||
| def orderedArgDefs = { | ||
| val impureArgs = typedArgBuf.filterNot { // pure are not lifted by liftArgs | ||
| case NamedArg(_, arg) => isPureExpr(arg) |
There was a problem hiding this comment.
Let's instead add a case for NamedArg in TreeInfo.exprPurity. That will avoid the special treatment here.
| typedArgs = liftArgs(liftedDefs, methType, liftable) ++ rest | ||
|
|
||
| // lift arguments in the definition order | ||
| val argDefBuff = mutable.ListBuffer.empty[Tree] |
There was a problem hiding this comment.
Usual naming convention in dotc is just Buf for buffer, i.e no double ff.
| case NamedArg(_, arg) => isPureExpr(arg) | ||
| case arg => isPureExpr(arg) | ||
| } | ||
| var defaultParamIndex = args.size |
| val argDefBuff = mutable.ListBuffer.empty[Tree] | ||
| typedArgs = liftArgs(argDefBuff, methType, typedArgs) | ||
|
|
||
| def orderedArgDefs = { |
There was a problem hiding this comment.
Can be val instead of def.
| val argDefBuff = mutable.ListBuffer.empty[Tree] | ||
| typedArgs = liftArgs(argDefBuff, methType, typedArgs) | ||
|
|
||
| def orderedArgDefs = { |
There was a problem hiding this comment.
Since the logic here is somewhat tricky, it would be good to add a comment with an example, what orderedArgDefs is supposed to achieve.
|
Ready for review |
|
test performance please |
|
performance test scheduled: 1 jobs in total. |
|
Performance test finished successfully: Visit https://liufengyun.github.io/bench/2931 to see the changes. |
This fixes the ordering issue described in #2916.
Additionally fixes another related issue where not all parameters that needed to be lifted where lifted. All these in presence of reordering done when having combination of effectful/pure arguments.