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.

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: use ptr_from_ref and ptr .cast() #4240

Merged
merged 2 commits into from
Jun 7, 2024

Conversation

davidhewitt
Copy link
Member

@davidhewitt davidhewitt commented Jun 7, 2024

Typically these days I like to use x.cast::<Y>() instead of x as *const Y to perform pointer casting (and in many contexts it's also convenient to let type inference work on just x.cast(), which reads a lot nicer than x as *const _). It also avoids const <-> mut casting by accident.

I just discovered ptr::from_ref and am now similarly excited to replace &x as *const X with ptr_from_ref(&x).

... so this PR is just a few tidy-ups across the codebase!

@davidhewitt davidhewitt added the CI-skip-changelog Skip checking changelog entry label Jun 7, 2024
@davidhewitt davidhewitt force-pushed the ptr-from-ref branch 2 times, most recently from 6cdbc75 to d6ea3c1 Compare June 7, 2024 16:26
Copy link
Member

@mejrs mejrs left a comment

Choose a reason for hiding this comment

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

Thanks, I much prefer this as well.

Also, we can use https://doc.rust-lang.org/std/primitive.pointer.html#method.cast_mut when we bump msrv next time :)

Copy link
Contributor

@Icxolu Icxolu left a comment

Choose a reason for hiding this comment

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

Very nice, I like this a lot!

@davidhewitt
Copy link
Member Author

Yes, as well as cast_mut there's ptr_from_mut, which I didn't try here (I think it's less relevant) but might also be handy a little.

@davidhewitt davidhewitt added this pull request to the merge queue Jun 7, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jun 7, 2024
@davidhewitt davidhewitt added this pull request to the merge queue Jun 7, 2024
Merged via the queue into PyO3:main with commit 9c67057 Jun 7, 2024
41 checks passed
@davidhewitt davidhewitt deleted the ptr-from-ref branch June 7, 2024 23:24
JRRudy1 added a commit to JRRudy1/pyo3 that referenced this pull request Jun 8, 2024
…tr_from_ref` added in PR PyO3#4240. Updated `PyRefMut::as_super` to use this method instead of `as *mut _`.
github-merge-queue bot pushed a commit that referenced this pull request Jun 9, 2024
* Added `PyRef::as_super` and `PyRefMut::as_super` methods, including docstrings and tests. The implementation of these methods also required adding `#[repr(transparent)]` to the `PyRef` and `PyRefMut` structs.

* Added newsfragment entry.

* Changed the `AsRef<U>`/`AsMut<U>` impls for `PyRef` and `PyRefMut` to use the new `as_super` methods. Added the `PyRefMut::downgrade` associated function for converting `&PyRefMut` to `&PyRef`. Updated tests and docstrings to better demonstrate the new functionality.

* Fixed newsfragment filename.

* Removed unnecessary re-borrows flagged by clippy.

* retrigger checks

* Updated `PyRef::as_super`, `PyRefMut::as_super`, and `PyRefMut::downgrade` to use `.cast()` instead of `as _` pointer casts. Fixed typo.

* Updated `PyRef::as_super` and `PyRefMut::downgrade` to use `ptr_from_ref` for the initial cast to `*const _` instead of `as _` casts.

* Added `pyo3::internal_tricks::ptr_from_mut` function alongside the `ptr_from_ref` added in PR #4240. Updated `PyRefMut::as_super` to use this method instead of `as *mut _`.

* Updated the user guide to recommend `as_super` for accessing the base class instead of `as_ref`, and updated the subsequent example/doctest to demonstrate this functionality.

* Improved tests for the `as_super` methods.

* Updated newsfragment to include additional changes.

* Fixed formatting.

---------

Co-authored-by: jrudolph <[email protected]>
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI-skip-changelog Skip checking changelog entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants