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

allow match in consts #6215

Closed
wants to merge 1 commit into from
Closed

allow match in consts #6215

wants to merge 1 commit into from

Conversation

llogiq
Copy link
Contributor

@llogiq llogiq commented Oct 24, 2020 •
edited
Loading

A followup to #6144, this extends our const handling to allow for match. Unsure how we'd test it, though, and we may want to extend it so we can add bindings to expressions in matches. Even so, I consider it a win.

r? @ebroto

changelog: none

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Oct 24, 2020
@llogiq
Copy link
Contributor Author

llogiq commented Oct 24, 2020 •
edited
Loading

Errors seem unrelated, and I cannot reproduce them. Perhaps a rebase will help.

@ebroto
Copy link
Member

ebroto commented Nov 4, 2020

Sorry for the delay in getting this reviewed, I'm really short on time lately. This will be the next PR I'll review when I get some free time.

@llogiq
Copy link
Contributor Author

llogiq commented Nov 7, 2020

No worries. I'm also just trying to catch up while following RustFest global...

ebroto
ebroto suggested changes Nov 10, 2020
View reviewed changes
Copy link
Member

@ebroto ebroto left a comment

Choose a reason for hiding this comment

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

I think we should add tests covering each new possible case in an arbitrary lint that uses the constant function.

clippy_lints/src/consts.rs
self.expr(matchee).and_then(|m| {
let outer_bindings = self.match_bindings.len();
for arm in arms {
self.match_bindings.truncate(outer_bindings);
Copy link
Member

Choose a reason for hiding this comment

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

I think this could be outside of the loop as outer_bindings will not change

@ebroto ebroto 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 Nov 10, 2020
@giraffate
Copy link
Contributor

ping from triage @llogiq. There seem some fixes left to be done. Could you have any update on this?

@llogiq
Copy link
Contributor Author

llogiq commented Nov 26, 2020

Yes, but I'll need some more time. If you want you can close and I can reopen when I get around to it.

@ebroto
Copy link
Member

ebroto commented Nov 26, 2020

Yes, but I'll need some more time. If you want you can close and I can reopen when I get around to it.

I know my requested changes are kind of problematic, sorry about that! I just want to make sure we cover those new paths to avoid triggering bugs that are hard to find because we don't test them.

Feel free to close the PR if you don't find time to adress them currently.

@ebroto
Copy link
Member

ebroto commented Dec 19, 2020

I'm going to close this for now to clean up a bit the PR queue

@ebroto ebroto closed this Dec 19, 2020
@ebroto ebroto added S-inactive-closed Status: Closed due to inactivity and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Dec 19, 2020
@flip1995
Copy link
Member

flip1995 commented Jan 7, 2021

@llogiq Can you move this branch to a fork, please? This is the only remaining branch, besides the necessary branches for CI, release, and deployment. cc #4745

@flip1995
Copy link
Member

I pushed this branch to my fork: https://github.com/flip1995/rust-clippy/tree/const-match

I now delete the branch from this repo. We now only have branches necessary for CI, release and deployment in this repo.

@flip1995 flip1995 deleted the const-match branch May 27, 2021 08:52
@flip1995 flip1995 mentioned this pull request May 27, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ebroto ebroto ebroto requested changes

Assignees

@ebroto ebroto

Labels
S-inactive-closed Status: Closed due to inactivity
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@llogiq @ebroto @giraffate @flip1995 @rust-highfive