Remove GenASM #4814
Remove GenASM #4814
Conversation
soc
force-pushed
the
topic/drop-genasm
branch
from
d401eca to
dfe7ee2
Compare
|
|
great! will take a look at it begin of next week. |
|
The failure is right, in the sense that we would generate invalid class files otherwise: I think the |
| @@ -1 +1 @@ | |||
| -Ybackend:GenASM | |||
|
|
|||
There was a problem hiding this comment.
the flags file is empty - just delete it :)
There was a problem hiding this comment.
Will do after I rebase this one on your fix for throws!
|
We discussed this at the Scala team meeting just now, and everyone is comfortable with going ahead with the removal for M4. |
|
@lrytz, @SethTisue thanks, great! |
soc
force-pushed
the
topic/drop-genasm
branch
from
dfe7ee2 to
effe179
Compare
|
Rebased! |
|
fingers crossed! |
|
@lrytz Did you/could you review it? |
|
sure, i will! |
| @@ -1 +1,26 @@ | |||
| bytecode identical | |||
There was a problem hiding this comment.
this diff output should not be in the check file. i'm not sure what's the best way to fix it - there's obviously no bug here, the only problem is that GenBCode assigns the local variable slots differently for methods a and b in https://github.com/scala/scala/blob/2.12.x/test/files/jvm/patmat_opt_no_nullcheck/Analyzed_1.scala.
I think the sameBytecode tests are a bit fragile anyway - we should instead just check for the actual properties (the tests file mentions "no null check"). I'm fine if you move this test to pending, I can take care of re-writing it.
There was a problem hiding this comment.
Argh, sorry, this was not intentional!
If I'm not mistaken, I saw that there is another method in the ASM API, something along the lines of "semantically equal". I'll try that before moving it to pending.
Edit: No, it's in partest-extras :-)
There was a problem hiding this comment.
Mhh, ASMConverters.equivalentBytecode didn't work (without digging any deeper).
I'll move it to pending for now.
|
otherwise it looks all good to me! |
|
link to the google doc summarizing test changes: https://docs.google.com/document/d/1P1oZEL3To0mMUx98Z6PKIaFHV_80kPhase0Zbn4699Q/edit |
With GenBCode being the default and only supported backend for Java 8, we can get rid of GenASM. This commit also fixes/migrates/moves to pending/deletes tests which depended on GenASM before.
soc
force-pushed
the
topic/drop-genasm
branch
from
effe179 to
1a8daa2
Compare
|
Moved the test to pending for now... |
|
@lrytz Is this good to go? I have trouble understanding "link to the google doc summarizing test changes:". Did you just note it, or wanted to say that I should include the link in the commit message? |
|
LGTM 🚮! |
|
🎉, thanks @soc! nothing feels better than deleting code |
Otherwise we lose the side effect of a `NegativeArraySizeException`. A test for this case already exists (run/t8601b.scala), but it currently enforces `-optimize -Ybackend:GenASM`, so it didn't trigger on the new backend. However, PR scala#4814 was merged into 2.12.x and moved that test over to the new backend and optimizer. After merging the 2.12.x into the current optimizer branch (push-pop elimination), the test started failing. Also disable the optimizer for `jvm/bytecode-test-example`: it counts the number of null checks in a method, the optimizer (rightly) eliminates one of the two.
Otherwise we lose the side effect of a `NegativeArraySizeException`. A test for this case already exists (run/t8601b.scala), but it currently enforces `-optimize -Ybackend:GenASM`, so it didn't trigger on the new backend. However, PR scala#4814 was merged into 2.12.x and moved that test over to the new backend and optimizer. After merging the 2.12.x into the current optimizer branch (push-pop elimination), the test started failing. Also disable the optimizer for `jvm/bytecode-test-example`: it counts the number of null checks in a method, the optimizer (rightly) eliminates one of the two.
No description provided.