-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Fix #876: Allow implicit conversions from singleton types #4738
Conversation
I found a way to make this efficient (hopefully).
test performance please |
performance test scheduled: 34 job(s) in queue, 1 running. |
When checking an implicit conversion for eligibility we should ignore any SingletonType upper bounds in its argument. This give rise to false negatives because we check against the underlying type when determining eligibility.
test performance please |
performance test scheduled: 34 job(s) in queue, 1 running. |
1 similar comment
performance test scheduled: 34 job(s) in queue, 1 running. |
Performance test finished successfully: Visit http://dotty-bench.epfl.ch/4738/ to see the changes. Benchmarks is based on merging with master (71f3aca) |
test performance please |
performance test scheduled: 30 job(s) in queue, 1 running. |
Performance test finished successfully: Visit http://dotty-bench.epfl.ch/4738/ to see the changes. Benchmarks is based on merging with master (bdf3ef4) |
@@ -71,17 +71,20 @@ object Implicits { | |||
/** The implicit references */ | |||
def refs: List[ImplicitRef] | |||
|
|||
private var SingletonClass: ClassSymbol = null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private[this]
to avoid generating accessors?
The benchmark numbers look good after tweaking. So this is ready to go in, from my side. |
I may be wrong but I don't think that "nor it" is grammatically correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise LGTM, nice to get this!
@@ -71,6 +71,15 @@ object Implicits { | |||
/** The implicit references */ | |||
def refs: List[ImplicitRef] | |||
|
|||
private var SingletonClass: ClassSymbol = null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why add a cache for SingletonClass?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same reason as for the caches in TypeComparer. It's used a lot and as a lazy val access is not super fast. It was one of the performance tweaks I applied after we saw a slip. Not sure it was the one that made the difference.
I found a way to make this efficient (hopefully).