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

Peel blocks and statements utils #8042

Merged
merged 3 commits into from
Dec 6, 2021
Merged

Peel blocks and statements utils #8042

merged 3 commits into from
Dec 6, 2021

Conversation

camsteffen
Copy link
Contributor

changelog: none

  • Rename remove_blocks to peel_blocks
  • Add peel_blocks_and_stmts
  • Various refactors to use the above utils
  • The utils also now check block.rules

@rust-highfive
Copy link

r? @xFrednet

(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 Nov 29, 2021
flip1995
flip1995 reviewed Nov 30, 2021
View reviewed changes
clippy_utils/src/lib.rs Outdated Show resolved Hide resolved
xFrednet
xFrednet reviewed Dec 3, 2021
View reviewed changes
clippy_utils/src/lib.rs Outdated
Comment on lines 1229 to 1230
/// Ie. `x`, `{ x }` and `{{{{ x }}}}` all give `x`. `{ x; y }` and `{}` return
/// themselves.
Copy link
Member

Choose a reason for hiding this comment

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

The implementation will also halt at blocks which contain a single statement. I think it's worth to mention this and provide an example. Also, to make clear what distinguishes this function from the other one. It would also be good to mention that this only peels normal blocks.

Maybe a table or something might be better than a sentence for the examples, even if this is still not perfect.

/// | Input           | Output        |
/// |-----------------|---------------|
/// | `x`             | `x`           |
/// | `{ x }`         | `x`           |
/// | `{{{{ x }}}}`   | `x`           |
/// | `{}`            | `{}`          |
/// | `{ y; x }`      | `{ y; x }`    |
/// | `{ y; }`        | `{ y; }`      |
/// | `{ {x}; }`      | `{ {x}; }`    |
/// | `{ unsafe {x} }`| `unsafe {x} ` |

clippy_lints/src/loops/manual_flatten.rs Outdated Show resolved Hide resolved
clippy_lints/src/needless_bool.rs Show resolved Hide resolved
@xFrednet
Copy link
Member

xFrednet commented Dec 3, 2021

The other changes look good to me, it's nice to remove some duplicate code! Good work 🙃

@xFrednet xFrednet added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Dec 3, 2021
@camsteffen
Copy link
Contributor Author

Rebased, renamed to peel_blocks_with_stmt, and improved docs. Let me know if you think the docs are clear enough.

@bors
Copy link
Contributor

bors commented Dec 6, 2021

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

@camsteffen camsteffen force-pushed the peel-blocks branch 2 times, most recently from 2a4ea57 to 551395e Compare December 6, 2021 15:45
xFrednet
xFrednet approved these changes Dec 6, 2021
View reviewed changes
Copy link
Member

@xFrednet xFrednet left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM, thank you for the changes! I would like to see one small change related to the documentation, then you can r=me 🙃

clippy_utils/src/lib.rs Outdated Show resolved Hide resolved
@camsteffen
Copy link
Contributor Author

Thanks for the review! @bors r=xFrednet

@bors
Copy link
Contributor

bors commented Dec 6, 2021

📌 Commit 973ec16 has been approved by xFrednet

@bors
Copy link
Contributor

bors commented Dec 6, 2021

⌛ Testing commit 973ec16 with merge d79919e...

bors added a commit that referenced this pull request Dec 6, 2021
@bors
Auto merge of #8042 - camsteffen:peel-blocks, r=xFrednet
d79919e 
Peel blocks and statements utils

changelog: none

* Rename `remove_blocks` to `peel_blocks`
* Add `peel_blocks_and_stmts`
* Various refactors to use the above utils
* The utils also now check `block.rules`
@camsteffen
Copy link
Contributor Author

@bors r-

@bors
Copy link
Contributor

bors commented Dec 6, 2021

💔 Test failed - checks-action_test

@camsteffen
Copy link
Contributor Author

@bors r+

(sorry, I accidentally had thrown in unrelated commits)

@bors
Copy link
Contributor

bors commented Dec 6, 2021

📌 Commit 16bbd24 has been approved by camsteffen

bors added a commit that referenced this pull request Dec 6, 2021
@bors
Auto merge of #8042 - camsteffen:peel-blocks, r=camsteffen
e8ef150 
Peel blocks and statements utils

changelog: none

* Rename `remove_blocks` to `peel_blocks`
* Add `peel_blocks_and_stmts`
* Various refactors to use the above utils
* The utils also now check `block.rules`
@bors
Copy link
Contributor

bors commented Dec 6, 2021

⌛ Testing commit 16bbd24 with merge e8ef150...

@camsteffen
Copy link
Contributor Author

@bors r-

@camsteffen
Copy link
Contributor Author

@bors r=xFrednet bah

@bors
Copy link
Contributor

bors commented Dec 6, 2021

📌 Commit 16bbd24 has been approved by xFrednet

@bors
Copy link
Contributor

bors commented Dec 6, 2021

⌛ Testing commit 16bbd24 with merge 48d939f...

@bors
Copy link
Contributor

bors commented Dec 6, 2021

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: xFrednet
Pushing 48d939f to master...

@bors bors merged commit 48d939f into rust-lang:master Dec 6, 2021
@bors bors mentioned this pull request Dec 6, 2021
Merged
@xFrednet
Copy link
Member

xFrednet commented Dec 6, 2021

That was a rollercoaster of emotions xD

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@flip1995 flip1995 flip1995 left review comments

@xFrednet xFrednet xFrednet approved these changes

Assignees

@xFrednet xFrednet

Labels
S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@camsteffen @rust-highfive @xFrednet @bors @flip1995