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

Don't lint match expressions with cfged arms #8443

Merged
merged 4 commits into from
Feb 21, 2022

Conversation

Jarcho
Copy link
Contributor

@Jarcho Jarcho commented Feb 17, 2022

Somehow there are no open issues related to this for any of the affected lints. At least none that I could fine from a quick search.

changelog: Don't lint match expressions with cfged arms in many cases

@rust-highfive
Copy link

r? @flip1995

(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 Feb 17, 2022
@flip1995
Copy link
Member

I'm not sure if re-lexing snippets on every match expression is a good idea. Also, every time I see something depending on so many span manipulations, I instantly think of ICEs.

@Jarcho
Copy link
Contributor Author

Jarcho commented Feb 18, 2022

This is only lexing everything in between each match arm, not the entire match expression. Lexing is also very quick. All of clippy_lints only takes half a second for me, including reading the files.

Doesn't snippet_opt not panic?

Copy link
Member

@flip1995 flip1995 left a comment

Choose a reason for hiding this comment

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

This is only lexing everything in between each match arm

Ah, then I misunderstood what the code does. I guess this shouldn't be too bad then. Giving this a bit more thought during the review, I think all the span operations are save. The code could use a few more comments though.

clippy_lints/src/matches/mod.rs Outdated Show resolved Hide resolved
clippy_lints/src/matches/mod.rs Show resolved Hide resolved
clippy_lints/src/matches/mod.rs Show resolved Hide resolved
clippy_lints/src/matches/mod.rs Show resolved Hide resolved
@flip1995 flip1995 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 Feb 18, 2022
@flip1995
Copy link
Member

Thanks for adding the comments!

Let's get this merged before Thursday, so we get this into nightly as long as possible. I'm always nervous about ICEs when it comes to span manipulation.

@bors r+

@bors
Copy link
Contributor

bors commented Feb 21, 2022

📌 Commit 78345b4 has been approved by flip1995

@bors
Copy link
Contributor

bors commented Feb 21, 2022

⌛ Testing commit 78345b4 with merge 9e605ef...

@bors
Copy link
Contributor

bors commented Feb 21, 2022

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: flip1995
Pushing 9e605ef to master...

@bors bors merged commit 9e605ef into rust-lang:master Feb 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants