-
Notifications
You must be signed in to change notification settings - Fork 1.8k
update borrow_as_ptr to suggest &raw syntax
#13689
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
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Jarcho (or someone else) some time within the next two weeks. Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (
|
b8a64f6 to
39a1dfc
Compare
Only the suggestion needs it (we can still fall back to the slightly older rust-clippy/clippy_config/src/msrvs.rs Line 22 in 786fbd6
and pass the |
|
Actually now looking more into it, I don't think this would ever do anything if gated behind an MSRV, because the lint |
|
You're right that it doesn't lint on current versions anymore, however, I fail to understand why this version restriction was added. For example, it should warn in this example and suggest #![warn(clippy::pedantic)]
fn main() {
let mut x = 2;
let ptr1 = &mut x as *mut i32;
let ptr2 = &mut x as *mut i32;
//let ptr1 = &raw mut x;
//let ptr2 = &raw mut x;
unsafe {
assert_eq!(*ptr1, *ptr2);
}
} |
|
That's nice and broken. @cyrgani can you make this two commits with the first flipping the order of Like @y21 mentioned you'll need the MSRV check for the suggestion. |
|
Note when flipping the order of the lints |
7d0b73a to
83241dd
Compare
clippy_lints/src/casts/mod.rs
Outdated
| let was_ptr_from_ref_emitted = if self.msrv.meets(msrvs::PTR_FROM_REF) { | ||
| ref_as_ptr::check(cx, expr, cast_from_expr, cast_to_hir) | ||
| } else { | ||
| false | ||
| }; | ||
| if self.msrv.meets(msrvs::BORROW_AS_PTR) && !was_ptr_from_ref_emitted { | ||
| borrow_as_ptr::check(cx, expr, cast_from_expr, cast_to_hir, &self.msrv); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You have the right idea here, but borrow_as_pointer should be checked first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, I've swapped the checks.
83241dd to
41ff29a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a couple of small things remaining and this is good.
| ) -> bool { | ||
| if matches!(cast_to.kind, TyKind::Ptr(_)) | ||
| && let ExprKind::AddrOf(BorrowKind::Ref, mutability, e) = cast_expr.kind | ||
| && let Some(std_or_core) = std_or_core(cx) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be moved down to where it's actually needed.
41ff29a to
c2ba0b3
Compare
c2ba0b3 to
9925f99
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you.
This PR updates the
borrow_as_ptrlint to no longer suggestaddr_of!andaddr_of_mut!and instead use the preferred&raw constand&raw mutsyntax.Not sure about two things:
borrow_as_ptr_no_stdtest as well as aborrow_as_ptrtest. They used to be more relevant as the lint needed to selectstdorcore, but that is gone now, so maybe theborrow_as_ptr_no_stdshould be deleted?changelog: update
borrow_as_ptrto suggest&rawsyntax