Fix #2924: make TreeChecker.typedBlock iterative #3080
Conversation
|
test performance please |
|
performance test scheduled: 1 job(s) in queue. |
|
Performance test finished successfully: Visit http://dotty-bench.epfl.ch/3080 to see the changes. Benchmarks is based on merge(s) with master |
nicolasstucki
force-pushed
the
improve-#2924
branch
from
5316c44 to
82196a5
Compare
|
test performance please |
|
performance test scheduled: 1 job(s) in queue. |
|
test performance please |
|
performance test scheduled: 1 job(s) in queue. |
|
Performance test finished successfully: Visit http://dotty-bench.epfl.ch/3080 to see the changes. Benchmarks is based on merge(s) with master |
| @@ -92,26 +92,27 @@ class FirstTransform extends MiniPhaseTransform with InfoTransformer with Annota | |||
| private def reorderAndComplete(stats: List[Tree])(implicit ctx: Context): List[Tree] = { | |||
| val moduleClassDefs, singleClassDefs = mutable.Map[Name, Tree]() | |||
|
|
|||
| def reorder(stats: List[Tree]): List[Tree] = stats match { | |||
| def reorder(stats: List[Tree], revPrefix: List[Tree] = Nil): List[Tree] = stats match { | |||
There was a problem hiding this comment.
This is a rather complex algorithm. Whenever I see reversed lists my head starts to hurt. Can we simplify it using ListBuffers? Also, if we keep it we need more docs to explain it.
There was a problem hiding this comment.
To clarify: I believe the original reorder algorithm was already close to the limit of tolerable complexity.
There was a problem hiding this comment.
Thinking more about it, I believe the revPrefix idea is the best option. So OK to leave the code as is. Just document what revPrefix stands for. I.e. at any point:
reordered statements = revPrefix.reverse ++ reorder(stats, Nil)
Also, I'd not make revPrefix have a default parameter. To easy to make errors that way.
| @@ -149,34 +149,42 @@ class TreeChecker extends Phase with SymTransformer { | |||
|
|
|||
| def withDefinedSym[T](tree: untpd.Tree)(op: => T)(implicit ctx: Context): T = tree match { | |||
| case tree: untpd.DefTree => | |||
| val sym = tree.symbol | |||
There was a problem hiding this comment.
I think the original logic was complicated - that foldRightBN is sneaky because it mixes laziness with side-effects ! The changes remove the foldRightBN complexity but make everything else more complex. Proposal: make withDefinedSyms take a repeated argument and remove withDefinedSym. I.e.
def withDefinedSyms[T](trees: untpd.Tree*)(op: => T)(implicit ctx: Context)
withDefinedSyms can step iteratively through all the trees, record the symbols found as defined in a ListBuffer or List and then execute op.
|
test performance please |
|
performance test scheduled: 1 job(s) in queue. |
|
Performance test finished successfully: Visit http://dotty-bench.epfl.ch/3080 to see the changes. Benchmarks is based on merge(s) with master |
|
test performance please |
|
performance test scheduled: 1 job(s) in queue. |
|
Performance test finished successfully: Visit http://dotty-bench.epfl.ch/3080 to see the changes. Benchmarks is based on merge(s) with master |
| case (stat: TypeDef) :: stats1 if stat.symbol.isClass => | ||
| if (stat.symbol is Flags.Module) { | ||
| def pushOnTop(xs: List[Tree], ys: List[Tree]): List[Tree] = xs match { |
There was a problem hiding this comment.
This one is a natural fold
(ys /: xs)((ys, x) => x :: ys)
There was a problem hiding this comment.
But it's also OK to leave as it is
This PR makes it posssible to compile arbitarley large blocks without growing the stack.