New lint: Recommend using ptr::eq when possible#4596
New lint: Recommend using ptr::eq when possible#4596tesuji wants to merge 1 commit intorust-lang:masterfrom
ptr::eq when possible#4596Conversation
JohnTitor
added
the
S-waiting-on-review
|
☔ The latest upstream changes (presumably #4592) made this pull request unmergeable. Please resolve the merge conflicts. |
|
☔ The latest upstream changes (presumably #4615) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Network errors in mac builder. |
|
☔ The latest upstream changes (presumably #4560) made this pull request unmergeable. Please resolve the merge conflicts. |
|
☔ The latest upstream changes (presumably #4683) made this pull request unmergeable. Please resolve the merge conflicts. |
|
☔ The latest upstream changes (presumably #4657) made this pull request unmergeable. Please resolve the merge conflicts. |
|
☔ The latest upstream changes (presumably #4788) made this pull request unmergeable. Please resolve the merge conflicts. |
|
☔ The latest upstream changes (presumably #4839) made this pull request unmergeable. Please resolve the merge conflicts. |
| LINT_MSG, | ||
| "try", | ||
| sugg, | ||
| Applicability::MachineApplicable, |
There was a problem hiding this comment.
How confident are you with the MachineApplicableility? I'M asking because of the formulation of
static LINT_MSG: &str = "use `std::ptr::eq` when applicable";There was a problem hiding this comment.
I've just added a macro check. But maybe there are more cases that I can't think of.
phansch
left a comment
phansch
left a comment
There was a problem hiding this comment.
I'm just learning about raw pointers, as I've never really used FFI before. Apologies if it sounds a bit pedantic but I genuinely don't know most of this yet 🙇♂️
clippy_lints/src/ptr_eq.rs
Outdated
| declare_clippy_lint! { | ||
| /// **What it does:** Use `std::ptr::eq` when applicable | ||
| /// | ||
| /// **Why is this bad?** This can be used to compare `&T` references |
There was a problem hiding this comment.
What's This, the first word, meant to reference? ptr::eq? If yes, I think it would read better as ptr::eq can be used to ...
| /// | ||
| /// **Why is this bad?** This can be used to compare `&T` references | ||
| /// (which coerce to `*const T` implicitly) by their address rather than | ||
| /// comparing the values they point to. |
There was a problem hiding this comment.
Is there another benefit in using ptr::eq, apart from being shorter?
phansch
added
S-waiting-on-author
|
☔ The latest upstream changes (presumably #4889) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Unfortunately, I don't think this lint is very useful. |
|
@lzutao I think this could still fit as a style lint, no? |
tesuji
force-pushed
the
lint-ptr-eq
branch
3 times, most recently
from
6fc0670 to
b5a9c82
Compare
Co-Authored-By: Phil Hansch <dev@phansch.net>
flip1995
added
S-waiting-on-review
|
☔ The latest upstream changes (presumably #4930) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Closed as inactive. |
flip1995
added
S-inactive-closed
New lint: Recommend using `ptr::eq` when possible This is based almost entirely on the code available in the previous PR #4596. I merely updated the code to make it compile. Fixes #3661. - [ ] I'm not sure about the lint name, but it was the one used in the original PR. - [X] Added passing UI tests (including committed `.stderr` file) - [X] `cargo test` passes locally - [X] Executed `cargo dev update_lints` - [X] Added lint documentation - [X] Run `cargo dev fmt` --- changelog: none
7 participants
changelog: Recommend using
ptr::eqwhen possibleCloses #3661