Fix #4866: do not lift try with no catch block #4872
Merged
Conversation
This file contains hidden or 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
allanrenucci
suggested changes
Jul 30, 2018
There was a problem hiding this comment.
Can you also add a test to i1692.scala. This prevented an optimisation on lazy vals that should be applicable now:
class LazyNullable {
...
private[this] val e = "E"
lazy val l4 = try e finally () // null out e
}
object Test {
...
def nullableTests() = {
...
assert(lz.l4 == "E")
assertNull("e")
...
}
...
}| * Lifting is needed only for try-catch expressions that are evaluated in a context | ||
| * where the stack might not be empty. `finally` does not attempt to continue evaluation | ||
| * after an exception, so the fact that values on the stack are 'lost' does not matter | ||
| * (copied from https://github.com/scala/scala/pull/922). |
There was a problem hiding this comment.
Indentation of the lines above is off by one character
tests/run/t4866.scala
Outdated
| @@ -0,0 +1,27 @@ | |||
| // Test that try-finally aren't lifted, but try-catch are. | |||
tests/run/t4866.scala
Outdated
| var lifted = 0 | ||
|
|
||
| for (f <- o.getClass.getDeclaredMethods) { | ||
| f.setAccessible(true) |
There was a problem hiding this comment.
Copypasta. Simplified.
tests/run/t4866.scala
Outdated
| def numLifted(o: Object): Int = { | ||
| var lifted = 0 | ||
|
|
||
| for (f <- o.getClass.getDeclaredMethods) { |
There was a problem hiding this comment.
def isLifted(method: Method) = method.getName.startsWith("lifted")
o.getClass.getDeclaredMethods.count(isLifted)
tests/run/t4866.scala
Outdated
| } | ||
|
|
||
| println("Foo #lifted: %d".format(numLifted(new Foo(new Object)))) | ||
| println("FooLifted #lifted: %d".format(numLifted(new FooLifted(new Object)))) |
There was a problem hiding this comment.
Why format?
"Foo #lifted: " + numLifted(new Foo(new Object))
tests/run/t4866.scala
Outdated
| @@ -0,0 +1,27 @@ | |||
| // Test that try-finally aren't lifted, but try-catch are. | |||
|
|
|||
| class Foo(param: => Object) { | |||
There was a problem hiding this comment.
Do we need a by-name parameter? Let's minimise feature interaction in tests:
class Foo {
def foo: Int = try ??? finally ()
}There was a problem hiding this comment.
We don't. Removed it.
tests/run/t4866.scala
Outdated
| val field = try param finally () | ||
| } | ||
|
|
||
| class FooLifted(param: => Object) { |
Forward port of scala/scala#922
|
PTAL |
allanrenucci
approved these changes
Jul 31, 2018
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.
Forward port of scala/scala#922