Skip to content

Split needless_borrow into two lints #11511

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

Merged
merged 2 commits into from
Sep 17, 2023
Merged

Conversation

Jarcho
Copy link
Contributor

@Jarcho Jarcho commented Sep 15, 2023

Splits off the case where the borrow is used as a generic argument to a function. I think the two cases are different enough to warrant a separate lint.

The tests for the new lint have been reordered to group related parts together. Two warning have been dropped, one looked like it was testing the generic argument form, but it ends up triggering the auto-deref variant. The second was just a redundant test that didn't do anything interesting.

An issue with cycle detection is also included. The old version was checking if a cycle was reachable from a block when it should have been checking if the block is part or a cycle.

As a side note, I'm liking the style of just jamming all the tests into separate scopes in main.

changelog: Split off needless_borrows_for_generic_args from needless_borrow

@rustbot
Copy link
Collaborator

rustbot commented Sep 15, 2023

r? @llogiq

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Sep 15, 2023
@Jarcho Jarcho force-pushed the split_borrow branch 3 times, most recently from 9e71b4d to b15a517 Compare September 15, 2023 22:59
@llogiq
Copy link
Contributor

llogiq commented Sep 16, 2023

The code looks good, but I wonder if we shouldn't name the new lint needless_generic_arg_borrow (without the s) to be more alike the needless_borrow lint. Alternative (but longer) would be needless_borrow_by_generic_arg.

@Jarcho
Copy link
Contributor Author

Jarcho commented Sep 17, 2023

needless_borrow really should plural according to the lint naming guidelines. needless_borrows_for_generic_args would work.

@llogiq
Copy link
Contributor

llogiq commented Sep 17, 2023

Thank you!

@bors r+

@bors
Copy link
Contributor

bors commented Sep 17, 2023

📌 Commit 79247d9 has been approved by llogiq

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Sep 17, 2023

⌛ Testing commit 79247d9 with merge 251a475...

@bors
Copy link
Contributor

bors commented Sep 17, 2023

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: llogiq
Pushing 251a475 to master...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants