-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Refactor ptr_offset_with_cast
#15613
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
Refactor ptr_offset_with_cast
#15613
Conversation
r? @Alexendoo rustbot has assigned @Alexendoo. Use |
Lintcheck changes for f981739
This comment will be updated if you push new changes |
Hmm this time rustbot is mistaken, I'm moving a lint here, not adding a new one. |
117f43c
to
cdca4f4
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.
Since this is a refactoring, some things might be made better as well.
For example:
ptr.offset(…)
is available since 1.0.0, whileptr.add(…)
is available only since 1.26.0. A MSRV check is necessary in order not to suggest unapplicable fixes.- Same happens with
ptr.wrapping_offset(…)
(1.16.0) andptr.wrapping_add(…)
(1.26.0).
Fortunately, all of those methods are const
since the same version, so no special handling is necessary for const
contexts.
Also, it would be preferable and more robust to use a multipart suggestion rather than building the new expression as a string:
- Get the method span and replace it with the new method name
- Compute the span of the
as …
part by truncating the cast expression span after the lhs and remove it
What do you think?
r? samueltardieu @rustbot author
Reminder, once the PR becomes ready for a review, use |
f1d9e61
to
5c0827f
Compare
That all sounds good to me. @rustbot ready |
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.
Looks good, except for the MSRV constant name which is too general. Also, could you add an example with &*const T
so that we can ensure that this is properly detected and that the suggestion is sound?
I have edited the top-level comment to include a proper changelog as it will be included in the merge commit. For reference, the previous content was:
|
5c0827f
to
4e2f5ea
Compare
Went with @rustbot ready |
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.
It looks good to me, except for the test which is not isolated (even though it is nice to see that this works fine with the double fix!).
4e2f5ea
to
f981739
Compare
@rustbot ready |
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.
Thanks!
Putting this lint in
/methods
is simpler and probably slightly more efficient.changelog: [
ptr_offset_with_cast
]: respect MSRV when suggesting fix, and lint even more cases