Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove GenASM #4814

Merged
merged 1 commit into from Oct 29, 2015
Merged

Remove GenASM #4814

merged 1 commit into from Oct 29, 2015

Conversation

@soc
Copy link
Member

@soc soc commented Oct 23, 2015

No description provided.

@scala-jenkins scala-jenkins added this to the 2.12.0-M4 milestone Oct 23, 2015
@soc soc force-pushed the soc:topic/drop-genasm branch from d401eca to dfe7ee2 Oct 23, 2015
@soc
Copy link
Member Author

@soc soc commented Oct 23, 2015

/home/jenkins/workspace/scala-2.12.x-validate-test/build-ant-macros.xml:776: Test suite finished with 1 case failing:
fail - run/t7008-scala-defined  [compilation failed]% scalac t7008-scala-defined/ScalaClassWithCheckedExceptions_1.scala
error: Error while emitting ScalaClassWithCheckedExceptions_1.scala
assertion failed: Cannot create ClassBType from non-class symbol type E1
one error found
@lrytz
Copy link
Member

@lrytz lrytz commented Oct 23, 2015

great! will take a look at it begin of next week.

@soc
Copy link
Member Author

@soc soc commented Oct 24, 2015

The failure is right, in the sense that we would generate invalid class files otherwise:

class C[E1 <: Exception] {
  @throws[E1]("") def bar() {}
}

I think the E1 type needs to be erased to a concrete type (the upper bound).

@@ -1 +1 @@
-Ybackend:GenASM

This comment has been minimized.

@lrytz

lrytz Oct 26, 2015
Member

the flags file is empty - just delete it :)

This comment has been minimized.

@soc

soc Oct 27, 2015
Author Member

Will do after I rebase this one on your fix for throws!

@lrytz
Copy link
Member

@lrytz lrytz commented Oct 26, 2015

@soc i submitted a fix in #4820, the test of this PR passes after applying it.

@SethTisue
Copy link
Member

@SethTisue SethTisue commented Oct 26, 2015

We discussed this at the Scala team meeting just now, and everyone is comfortable with going ahead with the removal for M4.

@soc
Copy link
Member Author

@soc soc commented Oct 27, 2015

@lrytz, @SethTisue thanks, great!

@soc soc force-pushed the soc:topic/drop-genasm branch from dfe7ee2 to effe179 Oct 27, 2015
@soc
Copy link
Member Author

@soc soc commented Oct 27, 2015

Rebased!

@lrytz
Copy link
Member

@lrytz lrytz commented Oct 27, 2015

fingers crossed!

@soc
Copy link
Member Author

@soc soc commented Oct 27, 2015

@lrytz Did you/could you review it?

@lrytz
Copy link
Member

@lrytz lrytz commented Oct 27, 2015

sure, i will!

@@ -1 +1,26 @@
bytecode identical

This comment has been minimized.

@lrytz

lrytz Oct 27, 2015
Member

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.

This comment has been minimized.

@soc

soc Oct 27, 2015
Author Member

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 :-)

This comment has been minimized.

@soc

soc Oct 27, 2015
Author Member

Mhh, ASMConverters.equivalentBytecode didn't work (without digging any deeper).

I'll move it to pending for now.

@lrytz
Copy link
Member

@lrytz lrytz commented Oct 27, 2015

otherwise it looks all good to me!

@lrytz
Copy link
Member

@lrytz lrytz commented Oct 27, 2015

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 soc force-pushed the soc:topic/drop-genasm branch from effe179 to 1a8daa2 Oct 27, 2015
@soc
Copy link
Member Author

@soc soc commented Oct 27, 2015

Moved the test to pending for now...

@soc
Copy link
Member Author

@soc soc commented Oct 28, 2015

@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?

@lrytz
Copy link
Member

@lrytz lrytz commented Oct 29, 2015

LGTM 🚮!

lrytz added a commit that referenced this pull request Oct 29, 2015
Remove GenASM
@lrytz lrytz merged commit 72538aa into scala:2.12.x Oct 29, 2015
5 checks passed
5 checks passed
cla @soc signed the Scala CLA. Thanks!
Details
integrate-ide [538] SUCCESS. Took 3 s.
Details
validate-main [642] SUCCESS. Took 153 min.
Details
validate-publish-core [628] SUCCESS. Took 11 min.
Details
validate-test [519] SUCCESS. Took 122 min.
Details
@SethTisue
Copy link
Member

@SethTisue SethTisue commented Oct 29, 2015

🎉, thanks @soc! nothing feels better than deleting code

lrytz added a commit to lrytz/scala that referenced this pull request Jan 24, 2016
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.
lrytz added a commit to lrytz/scala that referenced this pull request Jan 24, 2016
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.