Fix pattern matching alternative #5653
Conversation
andrevidela
force-pushed
the
fix-redundant-pattern
branch
from
72bf454 to
a007673
Compare
|
Actually, the unrechable code might be fixable in |
|
Thanks @andrevidela , it's OK to just fix it in this PR. |
andrevidela
force-pushed
the
fix-redundant-pattern
branch
from
a007673 to
a744ee5
Compare
andrevidela
force-pushed
the
fix-redundant-pattern
branch
from
a744ee5 to
ccb813a
Compare
|
@smarter I think this PR is ready, could you take a look? |
|
I just signed the CLA now |
| } | ||
|
|
||
| /** Flatten a list of patterns into a single tree */ | ||
| private def simplifyCases(alts: List[Tree]): Tree = alts match { |
| case alt :: Nil => alt | ||
| case Nil => Underscore(defn.IntType) // default case | ||
| case _ => Alternative(alts) | ||
| private def emitSwitchCases(cases: List[(List[Tree], Plan)]): List[CaseDef] = cases.foldLeft((List[CaseDef](), List[Tree]())) { |
There was a problem hiding this comment.
Generally our code style is to prefer tail-recursive formulation over folds and to replace Option with sentinels. This generally increases clarity and avoids unnecessary allocations. Allocations are your enemy - they make the compiler slow, more than anything else. And since they are spread out, there are no hot spots to address, you really have to take care to avoid them everywhere as much as is reasonable.
So would prefer the old formulation of emitCases over the new one.
| /** Remove cases that already appear in the same pattern or in previous patterns */ | ||
| private def removeRedundantCases(previousCases: List[Tree], cases: List[Tree]): List[Tree] = cases.foldLeft(List[Tree]()) { | ||
| case (cases, alt) => | ||
| if (cases.exists(_ === alt) || previousCases.exists(_ === alt)) { |
There was a problem hiding this comment.
This is quadratic in the number of cases. Maybe create a set of case values instead?
|
I had a quick look myself and came up with #5663, which is more conservative. I took the test case from this PR. So I think we can close this one now. Anyway, thank you for looking into it! |
Fix #5402
The same problem was fixed in scalac in 2013 in this pull request scala/scala#2291
I've reused the same test cases. Though there is one other issue
Will also crash the compiler with the same error described in #5402 . Compiling this code in scalac
results in a "code unreachable" warning. This suggest to me that this bug should be fixed in the code path analysis phase and not the switch case generation.
Should this problem be fixed in another pull request?