gh-149965: promote ptr_wise_atomic_memmove to internal header#151255
gh-149965: promote ptr_wise_atomic_memmove to internal header#151255clin1234 wants to merge 14 commits into
ptr_wise_atomic_memmove to internal header#151255Conversation
Commented out critical section assertion for ElementObject.
…FW3ZD.rst Co-authored-by: Sergey Miryanov <sergey.miryanov@gmail.com>
…ternal helper and add unit tests Extract the duplicated static `ptr_wise_atomic_memmove` from `Modules/_elementtree.c` and `Objects/listobject.c` into a shared `static inline _Py_ptr_wise_atomic_memmove(PyObject *a, ...)` in `Include/internal/pycore_object.h`. The first parameter is generalised from a type-specific pointer to `PyObject *` since only `PyObject *`- level operations (`_Py_IsOwnedByCurrentThread`, `_PyObject_GC_IS_SHARED`) were ever performed on it. `listobject.c` previously embedded `_Py_CRITICAL_SECTION_ASSERT_OBJECT_LOCKED` inside the local helper. That assertion is preserved by moving it explicitly to each of the four call sites in `listobject.c` (which are all called under a critical section). `_elementtree.c`'s open question about whether a critical section is needed remains unanswered, so no assertion is added there. Unit tests are added in `Modules/_testinternalcapi/test_ptr_wise_memmove.c` covering: dest < src (forward copy), dest > src (backward copy), dest == src (no-op), overlapping ranges, and the single-owner fast path. The single-owner test explicitly clears the GC SHARED bit to guard against freelist reuse leaving the bit set from a sibling test. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
clin1234
requested review from
a team,
AA-Turner,
emmatyping,
erlend-aasland and
itamaro
as code owners
|
@ericsnowcurrently, mind taking a look on helping me on resolving the mutable global variable issue in |
ZeroIntensity
left a comment
ZeroIntensity
left a comment
There was a problem hiding this comment.
I'm having a hard time understanding the motivation here. The DPO and C API WG threads look inconclusive.
There's not any reason to add tests and a generalized implementation for something that we only use in list; if we wanted to use it elsewhere, then we could move it to a header. If you have a plan to do so, then just do this in the PR you'll create. We don't need a separate patch just for moving the function.
There was a problem hiding this comment.
Internal changes don't need changelog entries; you can remove this.
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
…e-149965.edk2ut.rst
The reason for the generalized implementation is that I found it helpful for replacing certain |
|
I have made the requested changes; please review again. |
|
Thanks for making the requested changes! @ZeroIntensity: please review the changes made to this pull request. |
|
Ok, do the move in that fix. I don't personally think this needs unit tests beyond just covering the code paths that use it, but if you really want to have them, just put them in that PR. |
2 participants
See also capi-workgroup/decisions#106