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 ICode (re-submission) #4838

Merged
merged 6 commits into from Nov 10, 2015
Merged

Remove ICode (re-submission) #4838

merged 6 commits into from Nov 10, 2015

Conversation

@lrytz
Copy link
Member

@lrytz lrytz commented Nov 6, 2015

Re-submission of #4830. The first commit is the same, and it LGTM. For the rest, review by @soc.

soc and others added 2 commits Sep 16, 2015
@scala-jenkins scala-jenkins added this to the 2.12.0-M4 milestone Nov 6, 2015
@lrytz lrytz mentioned this pull request Nov 6, 2015
lrytz added 4 commits Nov 6, 2015
The only pieces of ICodes that were still used
  - An enum representing bytecode comparisons, re-implemented
  - The `icodes.IClass` class, which remains for sbt compatibility
    (#4588)
All class internal names that are referenced from a class being
compiled should be referenced through their ClassBType. This makes
sure that the ClassBType is cached in `classBTypeFromInternalName`,
which is required during classfile writing: when ASM computes stack
map frames, we need to answer subtyping queries, for which we need
to look up the ClassBTypes.
@lrytz lrytz force-pushed the lrytz:removeIcode branch from 148925d to aaf3ad7 Nov 6, 2015
@lrytz
Copy link
Member Author

@lrytz lrytz commented Nov 6, 2015

build is green now :)

@soc
Copy link
Member

@soc soc commented Nov 6, 2015

@lrytz Wow, great! Thanks for the cleanup, you solved all the stuff that I wasn't sure how/if it can be removed/merged. :-)

@lrytz
Copy link
Member Author

@lrytz lrytz commented Nov 9, 2015

@soc so LGTY? BTW, i was wondering if there were not any other tests that depend on ICode - did you rewrite / move to pending all of them in #4814?

@soc
Copy link
Member

@soc soc commented Nov 10, 2015

LGTM, sorry. :-) I think most ICode tests were already taken care of in #4814, but there weren't many to begin with ...

lrytz added a commit that referenced this pull request Nov 10, 2015
Remove ICode (re-submission)
@lrytz lrytz merged commit 44ae563 into scala:2.12.x Nov 10, 2015
4 checks passed
4 checks passed
integrate-ide [563] SUCCESS. Took 8 s.
Details
validate-main [672] SUCCESS. Took 103 min.
Details
validate-publish-core [659] SUCCESS. Took 7 min.
Details
validate-test [543] SUCCESS. Took 89 min.
Details
@lrytz
Copy link
Member Author

@lrytz lrytz commented Nov 10, 2015

Thanks for your work! I also finished re-writing the disabled tests, but that patch depends on getting #4837 in first, because that cleans up the redundant stores / loads you observed in https://github.com/scala/scala/pull/4814/files#diff-ec86a146d335b3385d7b4046aa4712fc.

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

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