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

New lint: Recommend using ptr::eq when possible #4596

Closed
wants to merge 1 commit into from
Closed

New lint: Recommend using ptr::eq when possible #4596

wants to merge 1 commit into from

Conversation

tesuji
Copy link
Contributor

@tesuji tesuji commented Sep 28, 2019

changelog: Recommend using ptr::eq when possible

Closes #3661

@JohnTitor JohnTitor added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Sep 28, 2019
@bors
Copy link
Contributor

bors commented Oct 8, 2019

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

@bors
Copy link
Contributor

bors commented Oct 9, 2019

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

@tesuji
Copy link
Contributor Author

tesuji commented Oct 9, 2019

Network errors in mac builder.

@bors
Copy link
Contributor

bors commented Oct 14, 2019

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

@bors
Copy link
Contributor

bors commented Oct 18, 2019

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

@bors
Copy link
Contributor

bors commented Oct 18, 2019

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

@bors
Copy link
Contributor

bors commented Nov 7, 2019

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

@bors
Copy link
Contributor

bors commented Nov 23, 2019

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

@flip1995 flip1995 added the A-lint Area: New lints label Nov 25, 2019
@tesuji tesuji closed this Nov 28, 2019
@tesuji tesuji reopened this Nov 28, 2019
flip1995
flip1995 reviewed Nov 28, 2019
View reviewed changes
clippy_lints/src/ptr_eq.rs
LINT_MSG,
"try",
sugg,
Applicability::MachineApplicable,
Copy link
Member

Choose a reason for hiding this comment

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

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";

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've just added a macro check. But maybe there are more cases that I can't think of.

phansch
phansch reviewed Dec 4, 2019
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.

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
Copy link
Member

Choose a reason for hiding this comment

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

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 ...

clippy_lints/src/ptr_eq.rs
///
/// **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.
Copy link
Member

Choose a reason for hiding this comment

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

Is there another benefit in using ptr::eq, apart from being shorter?

clippy_lints/src/ptr_eq.rs Outdated Show resolved Hide resolved
clippy_lints/src/ptr_eq.rs Outdated Show resolved Hide resolved
clippy_lints/src/ptr_eq.rs Outdated Show resolved Hide resolved
clippy_lints/src/ptr_eq.rs Outdated Show resolved Hide resolved
@phansch phansch 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 Dec 5, 2019
@bors
Copy link
Contributor

bors commented Dec 8, 2019

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

@tesuji
Copy link
Contributor Author

tesuji commented Dec 8, 2019

Unfortunately, I don't think this lint is very useful.
cc @matthiaskrgr for #4596 (comment) ?

@phansch
Copy link
Member

phansch commented Dec 9, 2019

@lzutao I think this could still fit as a style lint, no?

@tesuji tesuji force-pushed the lint-ptr-eq branch 3 times, most recently from 6fc0670 to b5a9c82 Compare December 10, 2019 04:31
@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 Dec 11, 2019
@bors
Copy link
Contributor

bors commented Dec 22, 2019

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

@tesuji
Copy link
Contributor Author

tesuji commented Dec 22, 2019

Closed as inactive.

@tesuji tesuji closed this Dec 22, 2019
@tesuji tesuji deleted the lint-ptr-eq branch December 22, 2019 10:13
@flip1995 flip1995 added S-inactive-closed Status: Closed due to inactivity and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Dec 22, 2019
@ambroisie ambroisie mentioned this pull request Oct 7, 2020
Merged
6 tasks
bors added a commit that referenced this pull request Oct 9, 2020
@bors
Auto merge of #6130 - Ambroisie:lint-ptr-eq, r=Manishearth
947516f 
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
@HKalbasi
Copy link
Member

finished in #6130
@rustbot label -S-inactive-closed

@rustbot rustbot removed the S-inactive-closed Status: Closed due to inactivity label Jul 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@flip1995 flip1995 flip1995 left review comments

@phansch phansch phansch left review comments

@matthiaskrgr matthiaskrgr Awaiting requested review from matthiaskrgr

Assignees
No one assigned
Labels
A-lint Area: New lints
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

new lint: suggest ptr::eq() instead of "x as *const _ as usize == y as *const as usize"
7 participants
@tesuji @bors @phansch @HKalbasi @flip1995 @JohnTitor @rustbot