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.

Sign up for GitHub

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

Jump to bottom

Cleanup rustc/ty part 1 #54717

Merged
merged 6 commits into from
Oct 6, 2018
Merged

Cleanup rustc/ty part 1 #54717

merged 6 commits into from
Oct 6, 2018

Conversation

ljedrz
Copy link
Contributor

@ljedrz ljedrz commented Oct 1, 2018 •
edited
Loading

Part 2 will follow soon; I wouldn't want these changes to rot too quickly.

  • improve stack shifting and remove related allocations
  • move a faster early return up
  • improve allocations
  • use Cow where applicable
  • simplify common patterns
  • whitespace fixes

@rust-highfive
Copy link
Collaborator

r? @michaelwoerister

(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 Oct 1, 2018
ljedrz
ljedrz commented Oct 1, 2018
View reviewed changes
src/librustc/ty/query/job.rs Outdated

// Zip them back together
let mut stack: Vec<_> = spans.into_iter().zip(queries).collect();
stack.0.swap(0, stack.len() - 1);
Copy link
Contributor Author

@ljedrz ljedrz Oct 1, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Whoops, I should have written stack.0.rotate_right(1) here; I'll fix it as soon as I get some CI results (in case of any other issues).

ljedrz
ljedrz commented Oct 1, 2018
View reviewed changes
src/librustc/ty/query/job.rs Outdated
// Shift the stack so that our entry point is first
let entry_point_pos = stack.iter().position(|(_, query)| query.as_ptr() == entry_point);
if let Some(pos) = entry_point_pos {
stack.shift_right(pos);
Copy link
Contributor Author

@ljedrz ljedrz Oct 1, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

That one should have been stack.rotate_right(pos); weird it checked fine I now noticed it's behind a compilation flag; I'll fix this shortly.

ljedrz
ljedrz commented Oct 1, 2018
View reviewed changes
src/librustc/ty/query/plumbing.rs Outdated
return jobs;
$(
jobs.extend(
self.$name.try_lock().unwrap().active.values().filter(|&v|
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This one is borked too due to me not noticing the compilation flag.

@rust-highfive

This comment has been minimized.

Sign in to view
@ljedrz

This comment has been minimized.

Sign in to view
@ljedrz

This comment has been minimized.

Sign in to view
@davidtwco

This comment has been minimized.

Sign in to view
@bors

This comment has been minimized.

Sign in to view
@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 Oct 3, 2018
@bors

This comment has been minimized.

Sign in to view
@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Oct 4, 2018
@ljedrz

This comment has been minimized.

Sign in to view
@davidtwco

This comment has been minimized.

Sign in to view
@bors

This comment has been minimized.

Sign in to view
@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 4, 2018
@bors
Copy link
Contributor

bors commented Oct 4, 2018

☔ The latest upstream changes (presumably #54809) made this pull request unmergeable. Please resolve the merge conflicts.

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Oct 4, 2018
@ljedrz

This comment has been minimized.

Sign in to view
@ljedrz ljedrz closed this Oct 4, 2018
@ljedrz ljedrz reopened this Oct 4, 2018
@ljedrz

This comment has been minimized.

Sign in to view
@davidtwco

This comment has been minimized.

Sign in to view
@bors

This comment has been minimized.

Sign in to view
@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 4, 2018
@bors
Copy link
Contributor

bors commented Oct 4, 2018

☔ The latest upstream changes (presumably #54649) made this pull request unmergeable. Please resolve the merge conflicts.

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Oct 4, 2018
@ljedrz
Copy link
Contributor Author

ljedrz commented Oct 5, 2018

Rebased.

@davidtwco
Copy link
Member

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Oct 5, 2018

📌 Commit 7ad21a8 has been approved by davidtwco

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 5, 2018
pietroalbini added a commit to pietroalbini/rust that referenced this pull request Oct 5, 2018
@pietroalbini
Rollup merge of rust-lang#54717 - ljedrz:cleanup_ty_p1, r=davidtwco
5efac03 
Cleanup rustc/ty part 1

Part 2 will follow soon; I wouldn't want these changes to rot too quickly.

- improve stack shifting and remove related allocations
- move a faster early return up
- improve allocations
- use Cow<str> where applicable
- simplify common patterns
- whitespace fixes
@pietroalbini pietroalbini mentioned this pull request Oct 5, 2018
Merged
bors added a commit that referenced this pull request Oct 6, 2018
@bors
Auto merge of #54859 - pietroalbini:rollup, r=pietroalbini
ac841e7 
Rollup of 11 pull requests

Successful merges:

 - #54078 (Expand the documentation for the `std::sync` module)
 - #54717 (Cleanup rustc/ty part 1)
 - #54781 (Add examples to `TyKind::FnDef` and `TyKind::FnPtr` docs)
 - #54787 (Only warn about unused `mut` in user-written code)
 - #54804 (add suggestion for inverted function parameters)
 - #54812 (Regression test for #32382.)
 - #54833 (make `Parser::parse_foreign_item()` return a foreign item or error)
 - #54834 (rustdoc: overflow:auto doesn't work nicely on small screens)
 - #54838 (Fix typo in src/libsyntax/parse/parser.rs)
 - #54851 (Fix a regression in 1.30 by reverting #53564)
 - #54853 (Remove unneccessary error from test, revealing NLL error.)

Failed merges:

r? @ghost
@bors bors merged commit 7ad21a8 into rust-lang:master Oct 6, 2018
@ljedrz ljedrz deleted the cleanup_ty_p1 branch October 6, 2018 06:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@davidtwco davidtwco

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ljedrz @rust-highfive @davidtwco @bors @michaelwoerister