-
Notifications
You must be signed in to change notification settings - Fork 783
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: update object.rs for 3.13 #4447
Conversation
65594c5
to
48c6a3b
Compare
Not sure how I got the commit message / PR title wrong originally; fat finger error while being hurried by family probably 🙈 |
48c6a3b
to
3863c96
Compare
pyo3-ffi/src/object.rs
Outdated
pub unsafe fn PyType_HasFeature(t: *mut PyTypeObject, f: c_ulong) -> c_int { | ||
(((*t).tp_flags & f) != 0) as c_int | ||
#[cfg(all(not(Py_LIMITED_API), Py_GIL_DISABLED))] | ||
let flags = (*ty).tp_flags.load(Ordering::Relaxed); |
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.
Good catch, totally missed this.
|
||
extern "C" { | ||
#[cfg(Py_3_13)] | ||
#[cfg_attr(PyPy, link_name = "PyPy_GetConstant")] |
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.
I guess there's no harm and it will probably work this way, but it seems a little premature to add code to support PyPy 3.13, which won't be coming out for quite a while, if ever.
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.
Yes I have gone back and forth on these "future" bindings for PyPy, but I've also had so many cases where the link_name
attribute has randomly missing creating compile errors for users on PyPy I just think it's slightly better this way round now.
arg1: *mut PyObject, | ||
arg2: *mut PyObject, | ||
arg3: *mut *mut PyObject, | ||
) -> c_int; |
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.
I don't know if the pattern of doing a getitem and then clearing the error afterwards shows up in the PyO3 codebase, but this will be a nice thing to use if it does show up. It's much faster to not create the error object if you don't actually need it.
let local = (*ob).ob_ref_local.load(Relaxed); | ||
if local == _Py_IMMORTAL_REFCNT_LOCAL { | ||
return _Py_IMMORTAL_REFCNT; | ||
#[cfg(Py_GIL_DISABLED)] |
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.
I like this pattern of exposing the function once and then using per-config blocks inside the function to do the per-config implementation. Much cleaner than having multiple copies of the signature. And it looks like it plays nicer with the doc build too.
@@ -123,38 +131,45 @@ pub struct PyVarObject { | |||
pub ob_size: Py_ssize_t, | |||
} | |||
|
|||
// skipped _PyVarObject_CAST | |||
// skipped private _PyVarObject_CAST | |||
|
|||
#[inline] | |||
pub unsafe fn Py_Is(x: *mut PyObject, y: *mut PyObject) -> c_int { | |||
(x == y).into() |
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.
Totally unrelated to this PR and a bit of a minor corner case, but this is wrong on pypy, see pypy/pypy#4044
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 for flagging, worth fixing anyway while I'm here!
9e7897b
to
6db725b
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.
Thanks; here is what I could review on my phone :)
pyo3-ffi/src/impl_/mod.rs
Outdated
@@ -0,0 +1,20 @@ | |||
#[cfg(Py_GIL_DISABLED)] | |||
mod atomic_c_ulong { | |||
pub(crate) struct GetAtomicCULong<const SIZE: usize>(); |
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.
Is the size in bits or in bytes? I would naively expect the latter, but have no opinion. Maybe we should call it "width" if we want the former.
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, I was thinking it's easier to think in bits here but size_of()
returns bytes, so this is currently bugged 👍
Renamed to WIDTH
👍
newsfragments/4447.added.md
Outdated
@@ -0,0 +1 @@ | |||
Add Python 3.13 FFI definitions `PyObject_GetOptionalAttr`, `PyObject_GetOptionalAttrString`, `PyObject_HasAttrWithError`, `PyObject_HasAttrStringWithError`, `Py_CONSTANT_*` constants, `Py_GetConstant`, `Py_GetConstantBorowed`, and `PyType_GetModuleByDef`. |
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.
Borrowed?
I will merge this to unblock forward progress; if there is more to do here then I can follow up. Thanks both for reviews! |
Co-authored-by: Alex Gaynor <[email protected]>
I'm not sure why the tests didn't catch it when this was merged, but
|
3.13 doesn't block merges yet, but you're right, I will investigate. |
Related to #4379, #4265
I audited
object.rs
for differences against 3.13 and applied all the differences I found here. At the same time I also removed all_Py
private APIs.I found a free threading detail in
PyType_HasFeature
which we needed to account for (atomic load), cc @ngoldbaum(Will push CHANGELOG later, out of time right now...)