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

If let else mutex #5332

Merged
merged 16 commits into from
Apr 20, 2020
Merged

If let else mutex #5332

merged 16 commits into from
Apr 20, 2020

Conversation

DevinR528
Copy link
Contributor

changelog: Adds lint to catch incorrect use of Mutex::lock in if let expressions with lock calls in any of the blocks.

closes: #5219

flip1995
flip1995 requested changes Mar 18, 2020
View reviewed changes
clippy_lints/src/if_let_mutex.rs Outdated Show resolved Hide resolved
clippy_lints/src/if_let_mutex.rs Outdated Show resolved Hide resolved
clippy_lints/src/if_let_mutex.rs Outdated 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) A-lint Area: New lints labels Mar 18, 2020
@DevinR528
Copy link
Contributor Author

I hope this works its definitely a lot cleaner, I removed if_chain, I wasn't sure how to avoid unnecessary walking and still use if_chain, but if the walking isn't that big of a deal I'll just call both of them up front and use if_chain.

@bors
Copy link
Collaborator

bors commented Mar 23, 2020

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

@bors
Copy link
Collaborator

bors commented Mar 30, 2020

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

@flip1995 flip1995 self-requested a review March 30, 2020 17:49
@bors
Copy link
Collaborator

bors commented Mar 30, 2020

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

@bors
Copy link
Collaborator

bors commented Apr 8, 2020

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

@flip1995 flip1995 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Apr 8, 2020
flip1995
flip1995 requested changes Apr 15, 2020
View reviewed changes
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.

9cdc7d5 added 2 binary files. Please remove them, preferably with a force push.

Sorry for taking so long for the review. I don't have an excuse, just wasn't really motivated to review PRs recently.

clippy_lints/src/if_let_mutex.rs Outdated Show resolved Hide resolved
doc/adding_lints.md Outdated Show resolved Hide resolved
clippy_lints/src/if_let_mutex.rs Outdated Show resolved Hide resolved
clippy_lints/src/if_let_mutex.rs Outdated Show resolved Hide resolved
tests/ui/if_let_mutex.rs Outdated 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 Apr 15, 2020
@bors
Copy link
Collaborator

bors commented Apr 15, 2020

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

@DevinR528
Copy link
Contributor Author

No worries I've seen you've been pretty busy getting the new release stuff and everything. Sorry I keep adding the binary files I wonder if there is a way to tell vscode when you open a non existent dir to just fail instead of creating a file.

@bors
Copy link
Collaborator

bors commented Apr 17, 2020

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

@bors
Copy link
Collaborator

bors commented Apr 19, 2020

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

phansch
phansch reviewed Apr 20, 2020
View reviewed changes
Copy link
Member

@phansch phansch left a comment

Choose a reason for hiding this comment

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

Implementation wise this looks good to me. Just some minor things =)

README.md Show resolved Hide resolved
clippy_lints/src/if_let_mutex.rs Outdated Show resolved Hide resolved
flip1995
flip1995 approved these changes Apr 20, 2020
View reviewed changes
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.

r=me after addressing @phansch comments and the test added

doc/adding_lints.md Outdated Show resolved Hide resolved
tests/ui/if_let_mutex.rs Show resolved Hide resolved
@phansch
Copy link
Member

phansch commented Apr 20, 2020

@bors r=flip1995

@bors
Copy link
Collaborator

bors commented Apr 20, 2020

📌 Commit 489dd2e has been approved by flip1995

bors added a commit that referenced this pull request Apr 20, 2020
@bors
Auto merge of #5332 - DevinR528:if-let-else-mutex, r=flip1995
023f3d1 
If let else mutex

changelog: Adds lint to catch incorrect use of `Mutex::lock` in `if let` expressions with lock calls in any of the blocks.

closes: #5219
@bors
Copy link
Collaborator

bors commented Apr 20, 2020

⌛ Testing commit 489dd2e with merge 023f3d1...

@DevinR528
Copy link
Contributor Author

How come the tests run sooo much faster? It's sweet.

@bors
Copy link
Collaborator

bors commented Apr 20, 2020

💔 Test failed - checks-action_test

@phansch
Copy link
Member

phansch commented Apr 20, 2020

Ah, you'll have to update the UI tests one more time because the line numbers changed.

How come the tests run sooo much faster? It's sweet.

I think that might be because of caching 🏃

@phansch
Copy link
Member

phansch commented Apr 20, 2020

@bors r=flip1995 thanks!

@bors
Copy link
Collaborator

bors commented Apr 20, 2020

📌 Commit 3fbe321 has been approved by flip1995

@bors
Copy link
Collaborator

bors commented Apr 20, 2020

⌛ Testing commit 3fbe321 with merge 6ce05bf...

@bors
Copy link
Collaborator

bors commented Apr 20, 2020

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

@bors bors merged commit 6ce05bf into rust-lang:master Apr 20, 2020
@bors bors mentioned this pull request Apr 20, 2020
Merged
@DevinR528
Copy link
Contributor Author

Thanks for the help and input as usual @flip1995 and @phansch!

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

@phansch phansch phansch left review comments

@flip1995 flip1995 flip1995 approved these changes

Assignees
No one assigned
Labels
A-lint Area: New lints 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.

[new-lint] if let Some... else with locked mutex
4 participants
@DevinR528 @bors @phansch @flip1995