-
Notifications
You must be signed in to change notification settings - Fork 13.5k
Use crate name for reexported extern crate
paths
#51017
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
Conversation
(rust_highfive has picked a reviewer for you, use r? to override) |
Tested locally with the ticket's repro, and it seems to be doing the right thing:
Need to figure out what the appropriate way to test this is. |
Ping from triage @michaelwoerister! This PR needs your review. |
Yes, a test would be great! |
Added test. |
📌 Commit 43d863b has been approved by |
⌛ Testing commit 43d863b with merge 8a3b0e33872bfb091d6bf352d183104d2dff6b8c... |
💔 Test failed - status-travis |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
1 similar comment
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
@bors retry |
Use crate name for reexported `extern crate` paths Fix #43189.
☀️ Test successful - status-appveyor, status-travis |
FWIW it looks like this had a fairly negative impact on perf for a few benchmarks |
@alexcrichton wow. Should we immediately revert this change? I'm surprised the performance regression is exclusively in |
I'm surprised this has any perf impact at all. |
http://perf.rust-lang.org/ shows a single NLL spike for this PR, then performance is immediately better again. Something might just have gone wrong with that performance measurement. I don't think this is something to worry about. |
Oh sorry I just wanted to bring this up as an FYI, no need to immediately revert! I just figured it'd be good to track if there's an easy or "obviously available" solution. @michaelwoerister it does indeed spike but I was suspicious of this comment because the commit afterwards was an explicit optimization for NLL. In that sense I thought this could be a regression followed immediately by an improvement and may still be worth looking into |
One interesting test though would be to submit a try build of reverting this PR and if it shows a performance improvement across the benchmarks that may mean there's something to look into! If it shows no change, however (as expected), then it could safely be filed under "spurious bug" |
Fix #43189.