Skip to content

Simplify Scala 2 trait support #6040

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

Merged
merged 1 commit into from
Mar 8, 2019

Conversation

smarter
Copy link
Member

@smarter smarter commented Mar 7, 2019

Drop vestigial code related to Scala 2.11 support. In particular, we
created a fake impl class in AugmentScala2Trait only to add its members
to the trait in LinkScala2Impls. We now directly add the members to the
trait in LinkScala2Impls.

We could potentially simplify things even further by getting rid of
LinkScala2Impls since the static foo$ methods in Scala 2 traits always
forward to instance methods foo, but that could have performance
implication as detailed in #5928, so we keep things as-is for now, but
eventually we should either switch Dotty trait encoding to also use
static forwarders, or not use them at all.

@smarter smarter requested a review from odersky March 7, 2019 16:02
@smarter
Copy link
Member Author

smarter commented Mar 7, 2019

test performance please

@dottybot
Copy link
Member

dottybot commented Mar 7, 2019

performance test scheduled: 1 job(s) in queue, 1 running.

@smarter smarter force-pushed the scala2-augment-after-erasure-2 branch from 2d12bf7 to 4739268 Compare March 7, 2019 16:05
Copy link
Member

@sjrd sjrd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes in backend/sjs/ LGTM.

@smarter smarter force-pushed the scala2-augment-after-erasure-2 branch 3 times, most recently from e2beab9 to 17e1d55 Compare March 7, 2019 16:29
@dottybot
Copy link
Member

dottybot commented Mar 7, 2019

Performance test finished successfully:

Visit http://dotty-bench.epfl.ch/6040/ to see the changes.

Benchmarks is based on merging with master (7d30a83)

Drop vestigial code related to Scala 2.11 support. In particular, we
created a fake impl class in AugmentScala2Trait only to add its members
to the trait in LinkScala2Impls. We now directly add the members to the
trait in LinkScala2Impls.

We could potentially simplify things even further by getting rid of
LinkScala2Impls since the static `foo$` methods in Scala 2 traits always
forward to instance methods `foo`, but that could have performance
implication as detailed in scala#5928, so we keep things as-is for now, but
eventually we should either switch Dotty trait encoding to also use
static forwarders, or not use them at all.
@smarter smarter force-pushed the scala2-augment-after-erasure-2 branch from 17e1d55 to d3de543 Compare March 8, 2019 10:23
Copy link
Contributor

@odersky odersky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice simplification

@odersky odersky merged commit 07847f8 into scala:master Mar 8, 2019
@nicolasstucki
Copy link
Contributor

@smarter this merge did not pass the test

@allanrenucci allanrenucci deleted the scala2-augment-after-erasure-2 branch March 8, 2019 19:02
smarter added a commit to dotty-staging/dotty that referenced this pull request Mar 8, 2019
Looks like scala#6034 and scala#6040 collided resulting in a CI failure on
master.
@smarter
Copy link
Member Author

smarter commented Mar 8, 2019

Fixed in #6049

@biboudis biboudis added this to the 0.14 Tech Preview milestone Apr 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants