Conversation
Tpt
force-pushed
the
tpt/ptr-eq
branch
3 times, most recently
from
fecf688 to
27ab224
Compare
| 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 |
davidhewitt
left a comment
davidhewitt
left a comment
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)
3 participants
Makes Clippy happy
Use also Into::into instead of explicit cast