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

rustc: remove Ty::{is_self, has_self_ty}. #53677

Closed
wants to merge 2 commits into from

Conversation

eddyb
Copy link
Member

@eddyb eddyb commented Aug 24, 2018

Fixes #50125 by replacing special-casing of Self (and the gensym hack) with getting the type for the Self parameter from the trait, and comparing types with that, instead.

NOTE: this is part of #53661, split out to independently check performance impact.

@rust-highfive
Copy link
Collaborator

r? @petrochenkov

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 24, 2018
@eddyb
Copy link
Member Author

eddyb commented Aug 24, 2018

r? @nikomatsakis

@bors try

@bors
Copy link
Contributor

bors commented Aug 24, 2018

⌛ Trying commit d110803 with merge 9a46196...

bors added a commit that referenced this pull request Aug 24, 2018
rustc: remove Ty::{is_self, has_self_ty}.

Fixes #50125 by replacing special-casing of `Self` (and the gensym hack) with getting the type for the `Self` parameter from the trait, and comparing types with that, instead.

**NOTE**: this is part of #53661, split out to independently check performance impact.
@bors
Copy link
Contributor

bors commented Aug 24, 2018

☀️ Test successful - status-travis
State: approved= try=True

@eddyb
Copy link
Member Author

eddyb commented Aug 24, 2018

@rust-timer build 9a46196

@rust-timer
Copy link
Collaborator

Success: Queued 9a46196 with parent 61b0072, comparison URL.

@eddyb
Copy link
Member Author

eddyb commented Aug 25, 2018

@nikomatsakis I'm surprised this makes a difference, but it's tiny. I'm going to push the next commit from #53661 (adding DefId to ParamTy) and see if that's to blame.

@eddyb

This comment has been minimized.

@bors

This comment has been minimized.

@eddyb
Copy link
Member Author

eddyb commented Aug 25, 2018

@bors try

@bors
Copy link
Contributor

bors commented Aug 25, 2018

⌛ Trying commit 79068b7 with merge 8bb91571ea5e75db22742d30e8ccca58b0bda2ee...

@bors
Copy link
Contributor

bors commented Aug 25, 2018

☀️ Test successful - status-travis
State: approved= try=True

@lqd

This comment has been minimized.

@eddyb
Copy link
Member Author

eddyb commented Aug 25, 2018

@rust-timer build 8bb91571ea5e75db22742d30e8ccca58b0bda2ee

@rust-timer
Copy link
Collaborator

Success: Queued 8bb91571ea5e75db22742d30e8ccca58b0bda2ee with parent f87d913, comparison URL.

@lqd
Copy link
Member

lqd commented Aug 25, 2018

Ok now perf results are ready :3

@eddyb
Copy link
Member Author

eddyb commented Aug 25, 2018

I wrote up the results in #53661 (comment), I'm closing this PR to keep things simple.

@eddyb eddyb closed this Aug 25, 2018
@eddyb eddyb deleted the not-my-self branch August 25, 2018 17:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants