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

proc_macro: Stay on the "use the cache" path more #50069

Merged
merged 1 commit into from
Apr 20, 2018

Conversation

alexcrichton
Copy link
Member

Discovered in #50061 we're falling off the "happy path" of using a stringified
token stream more often than we should. This was due to the fact that a
user-written token like 0xf is equality-different from the stringified token
of 15 (despite being semantically equivalent).

This patch updates the call to eq_unspanned with an even more awful solution,
probably_equal_for_proc_macro, which ignores the value of each token and
basically only compares the structure of the token stream, assuming that the AST
doesn't change just one token at a time.

While this is a step towards fixing #50061 there is still one regression
from #49154 which needs to be fixed.

Discovered in rust-lang#50061 we're falling off the "happy path" of using a stringified
token stream more often than we should. This was due to the fact that a
user-written token like `0xf` is equality-different from the stringified token
of `15` (despite being semantically equivalent).

This patch updates the call to `eq_unspanned` with an even more awful solution,
`probably_equal_for_proc_macro`, which ignores the value of each token and
basically only compares the structure of the token stream, assuming that the AST
doesn't change just one token at a time.

While this is a step towards fixing rust-lang#50061 there is still one regression
from rust-lang#49154 which needs to be fixed.
@alexcrichton
Copy link
Member Author

r? @nrc

@pietroalbini pietroalbini added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 19, 2018
@nrc
Copy link
Member

nrc commented Apr 19, 2018

Oh my.

@bors: r+ (but I hope you feel bad about this :-) )

@bors
Copy link
Contributor

bors commented Apr 19, 2018

📌 Commit e934873 has been approved by nrc

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 19, 2018
@alexcrichton
Copy link
Member Author

I do feel bad :(

This is indeed casting a shadow of a doubt on macros 1.2...

@bors
Copy link
Contributor

bors commented Apr 20, 2018

⌛ Testing commit e934873 with merge 257d43d...

bors added a commit that referenced this pull request Apr 20, 2018
proc_macro: Stay on the "use the cache" path more

Discovered in #50061 we're falling off the "happy path" of using a stringified
token stream more often than we should. This was due to the fact that a
user-written token like `0xf` is equality-different from the stringified token
of `15` (despite being semantically equivalent).

This patch updates the call to `eq_unspanned` with an even more awful solution,
`probably_equal_for_proc_macro`, which ignores the value of each token and
basically only compares the structure of the token stream, assuming that the AST
doesn't change just one token at a time.

While this is a step towards fixing #50061 there is still one regression
from #49154 which needs to be fixed.
@bors
Copy link
Contributor

bors commented Apr 20, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: nrc
Pushing 257d43d to master...

@bors bors merged commit e934873 into rust-lang:master Apr 20, 2018
@alexcrichton alexcrichton deleted the fix-proc-macro branch May 10, 2018 19:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants