Fix #856: Handle try/catch cases as catch cases if possible. #1315
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Review by @DarkDimius |
nicolasstucki
force-pushed
the
optimize-try-cases
branch
from
9a7df53 to
8e82089
Compare
|
|
||
| /** Is this pattern node a catch-all or type-test pattern? */ | ||
| private def isCatchCase(cdef: CaseDef)(implicit ctx: Context): Boolean = cdef match { | ||
| case CaseDef(Typed(Ident(nme.WILDCARD), tpt), _, _) => isSimpleThrowable(tpt.tpe) |
There was a problem hiding this comment.
what about guards? Since this PR they will not be desugared by patmat, and will reach the backend and be ignored there.
|
@nicolasstucki please ping me when ready for next review round. Thanks. |
| * try { <code> } | ||
| * catch { | ||
| * <tryCases> // Cases that can be handled by catch | ||
| * <paterMatchCases> // Cases starting with first one that can't be handled by catch |
There was a problem hiding this comment.
Oh, same elsewhere, so I guess it's not a typo but intentional, still seems a bit weird
There was a problem hiding this comment.
It was indeed a typo. Must have copy pasted it around.
nicolasstucki
force-pushed
the
optimize-try-cases
branch
2 times, most recently
from
a48f1df to
bcda17d
Compare
|
@DarkDimius this PR is ready for another round of review. |
| override def checkPostCondition(tree: Tree)(implicit ctx: Context): Unit = tree match { | ||
| case Try(_, cases, _) => | ||
| cases.foreach { | ||
| case CaseDef(Typed(_, _), guard, _) => assert(guard.isEmpty, "Try case should not a guard.") |
There was a problem hiding this comment.
A synonym of have is missing. Same on the next line.
Previously they were all lifted into a match with the came cases. Now the first cases are handled directly by by the catch. If one of the cases can not be handled the old scheme is applied to to it and all subsequent cases.
nicolasstucki
force-pushed
the
optimize-try-cases
branch
from
bcda17d to
b0ebe6a
Compare
|
LGTM, Thanks! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously they were all lifted into a match with the came cases.
Now the first cases are handled directly by by the catch. If one
of the cases can not be handled the old scheme is applied to to it
and all subsequent cases.