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

Fix all occurences needless_borrow internally #6931

Merged
merged 1 commit into from
Apr 6, 2021

Conversation

Jarcho
Copy link
Contributor

@Jarcho Jarcho commented Mar 18, 2021

The bug that got 'needless_borrow' moved into the nursery was fixed two years ago in d4370f8.

This did trigger over a thousand times internally, so that's all the other changes. I vetted most of them, but there's a lot The only interesting change is to the lint list. declare_tool_lint already makes a reference, so there's no need to take a reference to the lints.

changelog: None

@rust-highfive
Copy link

r? @phansch

(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 Mar 18, 2021
@Jarcho Jarcho changed the title Move needless_borrow from the nursery. The bug that moved it there … Move needless_borrow from the nursery. Mar 18, 2021
@phansch
Copy link
Member

phansch commented Mar 22, 2021

@Jarcho Did you happen to run lintcheck to see if there were any false positives or similar amounts of triggers?

I'm currently a bit hesitant to merge this because it caused so many changes in our codebase alone.

@bors
Copy link
Collaborator

bors commented Mar 22, 2021

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

@Jarcho
Copy link
Contributor Author

Jarcho commented Mar 22, 2021

Not bad overall

  • rayon: 7
  • regex: 1
  • ripgrep: 2
  • syn: 1
  • tam-oidc: 3
  • xsv: 3

Pretty much every instance in clippy is from taking a reference to a HIR node. Those are mostly all references to start with.

@bors
Copy link
Collaborator

bors commented Mar 23, 2021

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

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.

The changes LGTM and I would merge them like this after a rebase.

But I would do the actual move of the lint in another PR. That PR should then add tests for the remaining open issues about this lint to verify that they're fixed or to at least know the state this lint is in.

@flip1995
Copy link
Member

flip1995 commented Apr 1, 2021

This is a great code cleanup btw! Thanks for all the work you put into this!

@Jarcho
Copy link
Contributor Author

Jarcho commented Apr 1, 2021

I'll pull out the category change when I rebase it. There's still the bad suggestion with pattern references to deal with. The rest of those are just enhancements.

@Jarcho Jarcho changed the title Move needless_borrow from the nursery. Fix all occurences needless_borrow internally Apr 2, 2021
@bors
Copy link
Collaborator

bors commented Apr 5, 2021

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

@bors
Copy link
Collaborator

bors commented Apr 5, 2021

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

@flip1995
Copy link
Member

flip1995 commented Apr 6, 2021

@Jarcho Sorry that we missed your rebase. Please self-approve this after rebasing again.

@bors delegate+ p=10

@bors
Copy link
Collaborator

bors commented Apr 6, 2021

✌️ @Jarcho can now approve this pull request

@flip1995
Copy link
Member

flip1995 commented Apr 6, 2021

@bors r=phansch,flip1995

Thanks!

@bors
Copy link
Collaborator

bors commented Apr 6, 2021

📌 Commit 12fce55 has been approved by phansch,flip1995

@bors
Copy link
Collaborator

bors commented Apr 6, 2021

⌛ Testing commit 12fce55 with merge 6ae0835...

@Jarcho
Copy link
Contributor Author

Jarcho commented Apr 6, 2021

Looks like you got to it before me.

@bors
Copy link
Collaborator

bors commented Apr 6, 2021

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

@bors bors merged commit 6ae0835 into rust-lang:master Apr 6, 2021
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.

5 participants