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

Refactor the MirPass for QualifyAndPromoteConstants #63994

Merged
merged 8 commits into from
Sep 8, 2019

Conversation

Centril
Copy link
Contributor

@Centril Centril commented Aug 29, 2019

This is an accumulation of drive-by commits while working on Vec::new as a stable const fn.
The PR is probably easiest read commit-by-commit.

r? @oli-obk

cc @eddyb @ecstatic-morse -- your two PRs #63812 and #63860 respectively will conflict with this a tiny bit but it should be trivial to reintegrate your changes atop of this.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 29, 2019
@oli-obk
Copy link
Contributor

oli-obk commented Aug 29, 2019

This PR will conflict heavily with the mentioned two PRs. I'd like to block it on getting the others merged

@Centril
Copy link
Contributor Author

Centril commented Aug 29, 2019

@oli-obk I checked the diff and it will only conflict with each of those PRs in 3-7 lines at most

@bors

This comment has been minimized.

@Centril
Copy link
Contributor Author

Centril commented Aug 31, 2019

Rebased & simplified things a bit.

@JohnCSimon
Copy link
Member

Ping from triage:
@oli-obk @varkor @spastorino
Hi!
Can you please review this PR?
Thanks

@Centril
Copy link
Contributor Author

Centril commented Sep 8, 2019

r? @spastorino

@rust-highfive rust-highfive assigned spastorino and unassigned oli-obk Sep 8, 2019
@oli-obk
Copy link
Contributor

oli-obk commented Sep 8, 2019

@bors r=spastorino,oli-obk

@bors
Copy link
Contributor

bors commented Sep 8, 2019

📌 Commit 0a8a3dd has been approved by spastorino,oli-obk

@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 Sep 8, 2019
@bors
Copy link
Contributor

bors commented Sep 8, 2019

⌛ Testing commit 0a8a3dd with merge 2b8116d...

bors added a commit that referenced this pull request Sep 8, 2019
…oli-obk

Refactor the `MirPass for QualifyAndPromoteConstants`

This is an accumulation of drive-by commits while working on `Vec::new` as a stable `const fn`.
The PR is probably easiest read commit-by-commit.

r? @oli-obk

cc @eddyb @ecstatic-morse -- your two PRs #63812 and #63860 respectively will conflict with this a tiny bit but it should be trivial to reintegrate your changes atop of this.
@bors
Copy link
Contributor

bors commented Sep 8, 2019

☀️ Test successful - checks-azure
Approved by: spastorino,oli-obk
Pushing 2b8116d to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 8, 2019
@bors bors merged commit 0a8a3dd into rust-lang:master Sep 8, 2019
@Centril Centril deleted the refactor-qualify-consts branch September 8, 2019 20:34
fn remove_drop_and_storage_dead_on_promoted_locals(
body: &mut Body<'tcx>,
promoted_temps: &BitSet<Local>,
) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw, according to @RalfJung and @oli-obk's discussions elsewhere(?), we shouldn't even be doing this (but instead run regular promotion).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Open an issue?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is from another issue, but I don't remember which one, heh.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure about the "should" here, I was just very surprised to learn that this is how promotion works.

The discussion was at #63955 (comment).

@eddyb
Copy link
Member

eddyb commented Sep 12, 2019

Would be nice if the PR author could toggle the default "whitespace" diff setting (the diff is way more readable with "No whitespace").

@Centril
Copy link
Contributor Author

Centril commented Sep 12, 2019

Would be nice if the PR author could toggle the default "whitespace" diff setting (the diff is way more readable with "No whitespace").

Wow; I just looked now. You are so right.

@RalfJung
Copy link
Member

Would be nice if the PR author could toggle the default "whitespace" diff setting (the diff is way more readable with "No whitespace").

AFAIK this is a setting you pick, not something the PR author sets? I can toggle this behind the gear icon in the diff view.

@Centril
Copy link
Contributor Author

Centril commented Sep 14, 2019

@RalfJung Yeah I believe @eddyb was making a "feature request" to GitHub...

@RalfJung
Copy link
Member

Oh I see. I read it as asking the PR author here to do something.^^

@Ixrec
Copy link
Contributor

Ixrec commented Sep 14, 2019

Not a great workaround, but: In addition to the setting, you can add ?w=1 to the /files url. So whenever I have a PR where hiding whitespace helps a lot, or only some commits should be reviewed because the others are autogenerated stuff or whatever, I usually put a "Recommend reviewing without whitespace" link at the top of my PR description. I think I got the idea from Rust RFC PRs always having a "Rendered" link.

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.

9 participants