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

ffi: define compat for Py_NewRef and Py_XNewRef #4445

Merged
merged 5 commits into from
Aug 17, 2024

Conversation

davidhewitt
Copy link
Member

This improves the FFI definitions Py_NewRef and Py_XNewRef in the following ways:

  • Adds #[doc(cfg(Py_3_10))] to the original definitions (which are either an extern symbol or an inline function depending on abi3 setting, and so would a cfg hint in the doc saying they were only available in one of these)
  • Adds Py_NewRef and Py_XNewRef to pyo3_ffi::compat, so we can use them on all versions internally
  • Removes the private implementation details _Py_NewRef and _Py_XNewRef

To make the doc-cfgs on pyo3_ffi::compat work properly, I realised that I needed to use functions rather than re-exports when running the docrs build. I decided to use a macro to make this implementation easier. This way the compat functions are identical regardless of what Python version we build with. (cc @mejrs)

This is what I saw before (clicking on these re-exports then shows a version cfg):

Screenshot 2024-08-16 143146

After:

Screenshot 2024-08-16 152258

davidhewitt and others added 2 commits August 16, 2024 21:19
Co-authored-by: Nathan Goldbaum <nathan.goldbaum@gmail.com>
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 think we should add a test to ensure glob re-exports do not trigger import ambiguity:

mod test_export{
     pub use pyo3_ffi::compat::*;
     pub use pyo3_ffi::*;
}

@davidhewitt
Copy link
Member Author

Great idea. It turned out to be a little bit harder than just that pair of use statements, because to trigger the ambiguity error I actually needed to use an ambiguous name. What I ended up doing was adding a test to the code generated from the compat_function! macro so that it automatically set up a check. This should automatically ensure new symbols in ffi::compat are never ambiguous 😄

@davidhewitt davidhewitt enabled auto-merge August 16, 2024 22:04
@davidhewitt davidhewitt added this pull request to the merge queue Aug 16, 2024
github-merge-queue bot pushed a commit that referenced this pull request Aug 16, 2024
* ffi: define compat for `Py_NewRef` and `Py_XNewRef`

* add missing inline hint

Co-authored-by: Nathan Goldbaum <nathan.goldbaum@gmail.com>

* don't use std::ffi::c_int (requires MSRV 1.64)

* add test to guard against ambiguity

---------

Co-authored-by: Nathan Goldbaum <nathan.goldbaum@gmail.com>
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 16, 2024
@davidhewitt davidhewitt enabled auto-merge August 17, 2024 07:17
@davidhewitt davidhewitt added this pull request to the merge queue Aug 17, 2024
Merged via the queue into PyO3:main with commit 144b199 Aug 17, 2024
43 checks passed
@davidhewitt davidhewitt deleted the dh/compat-pynewref branch August 17, 2024 08:06
davidhewitt added a commit that referenced this pull request Sep 3, 2024
* ffi: define compat for `Py_NewRef` and `Py_XNewRef`

* add missing inline hint

Co-authored-by: Nathan Goldbaum <nathan.goldbaum@gmail.com>

* don't use std::ffi::c_int (requires MSRV 1.64)

* add test to guard against ambiguity

* fix `Py_NewRef` cfg on PyPy

---------

Co-authored-by: Nathan Goldbaum <nathan.goldbaum@gmail.com>
davidhewitt added a commit that referenced this pull request Sep 3, 2024
* ffi: define compat for `Py_NewRef` and `Py_XNewRef`

* add missing inline hint

Co-authored-by: Nathan Goldbaum <nathan.goldbaum@gmail.com>

* don't use std::ffi::c_int (requires MSRV 1.64)

* add test to guard against ambiguity

* fix `Py_NewRef` cfg on PyPy

---------

Co-authored-by: Nathan Goldbaum <nathan.goldbaum@gmail.com>
davidhewitt added a commit that referenced this pull request Sep 15, 2024
* ffi: define compat for `Py_NewRef` and `Py_XNewRef`

* add missing inline hint

Co-authored-by: Nathan Goldbaum <nathan.goldbaum@gmail.com>

* don't use std::ffi::c_int (requires MSRV 1.64)

* add test to guard against ambiguity

* fix `Py_NewRef` cfg on PyPy

---------

Co-authored-by: Nathan Goldbaum <nathan.goldbaum@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants