Makes Clippy beta happy #5032
Makes Clippy beta happy #5032
Conversation
Tpt
force-pushed
the
tpt/ptr-eq
branch
3 times, most recently
from
fecf688 to
27ab224
Compare
pyo3-macros-backend/src/method.rs
Outdated
| @@ -53,6 +53,7 @@ pub struct PyArg<'a> { | |||
| pub ty: &'a syn::Type, | |||
| } | |||
|
|
|||
| #[allow(clippy::large_enum_variant)] | |||
There was a problem hiding this comment.
Change to make Clippy happy
There was a problem hiding this comment.
We should consider addressing it. It seems that RegularArg is the big one here. We can probably put it or most of its fields behind a reference like the other variants.
Anyway that shouldn't block this PR, but please file an issue for it if we merge this as is.
There was a problem hiding this comment.
We can probably put it or most of its fields behind a reference like the other variants.
Yes, but it does not seems trivial and will require some refactoring. I have open #5039 about it.
Anyway, this is a structure we do not keep in memory for a while, I am not sure how much we care about its size.
|
Is this a clippy beta bug? It looks to me like this probably came from rust-lang/rust-clippy#14339 But this seems like a lot of ptr equality here is being needlessly modified when there is no references involved. Reference to ptr coercion seemed to be the original motivation of the lint... https://rust-lang.github.io/rust-clippy/stable/index.html#ptr_eq Maybe we should report upstream? |
|
Filed rust-lang/rust-clippy#14525, let's see the outcome of that... |
|
I'd prefer to globally allow this lint. I find the increase in parentheses make it harder to read. |
|
I agree, at least for |
|
Thank you for your feedback. I have reverted the changes in pyo3-ffi and allowed there the
Yes! However, imho there is still value to use Anyway, happy to revert the changes in |
There was a problem hiding this comment.
Yes! However, imho there is still value to use
ptr::eqinstead of==: it makes cristal clear we are using pointer equality and not value equality via the PartialEq trait. If it's not obvious that arguments are pointers it increases readability.
I guess that's a reasonable argument. TBH I'm used to the fact that pointers don't do value equality, but I can see why clippy might want to encourage users to think about that.
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
* Use std::ptr::eq where relevant * allow(clippy::ptr_eq) in pyo3-ffi * Add ref for allow(clippy::large_enum_variant)
* Use std::ptr::eq where relevant * allow(clippy::ptr_eq) in pyo3-ffi * Add ref for allow(clippy::large_enum_variant)
Makes Clippy happy
Use also Into::into instead of explicit cast