Skip to content

Add note about reverting a workaround in the future #84063

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
Apr 11, 2021

Conversation

LingMan
Copy link
Contributor

@LingMan LingMan commented Apr 10, 2021

The root cause was fixed upstream in LLVM main. This adds a reminder to revert the workaround once the LLVM rustc depends on is new enough. Since I'm not sure how such optimizations get routed to LLVM releases, I used the conservative assumption that it will only show up with LLVM 13.

The root cause was fixed upstream in LLVM main. This adds a reminder to revert the workaround once the LLVM rustc depends on is new enough. Since I'm not sure how such optimizations get routed to LLVM releases, I used the conservative assumption that it will only show up with LLVM 13.
@rust-highfive
Copy link
Contributor

r? @dtolnay

(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 Apr 10, 2021
@nagisa
Copy link
Member

nagisa commented Apr 10, 2021

Why is it important that we remove this workaround? Does the workaround produce worse code than what the reverted code with updated LLVM would?

@LingMan
Copy link
Contributor Author

LingMan commented Apr 10, 2021

"Important" isn't the word I'd use. It's just written in a slightly non-obvious way that triggered even the author of the workaround to add an explanatory comment. Plus, a future reader might try to figure out what's up with the "optimizes worse" part of the comment when it has been long fixed. My goal is to inform them that there's a known root cause fix and when it will be safe to simplify the code again.

@nagisa
Copy link
Member

nagisa commented Apr 10, 2021

Cool.

@bors r+ rollup=always

@bors
Copy link
Collaborator

bors commented Apr 10, 2021

📌 Commit 9aa11a1 has been approved by nagisa

@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 10, 2021
@dtolnay dtolnay assigned nagisa and unassigned dtolnay Apr 10, 2021
@bors
Copy link
Collaborator

bors commented Apr 11, 2021

⌛ Testing commit 9aa11a1 with merge 58f32da...

@bors
Copy link
Collaborator

bors commented Apr 11, 2021

☀️ Test successful - checks-actions
Approved by: nagisa
Pushing 58f32da to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 11, 2021
@bors bors merged commit 58f32da into rust-lang:master Apr 11, 2021
@rustbot rustbot added this to the 1.53.0 milestone Apr 11, 2021
@LingMan LingMan deleted the patch-1 branch April 11, 2021 13:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. 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.

6 participants