FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

gh-149965: promote `ptr_wise_atomic_memmove` to internal header by clin1234 · Pull Request #151255 · python/cpython · GitHub

/ cpython Public

gh-149965: promote ptr_wise_atomic_memmove to internal header - #151255

Closed
clin1234 wants to merge 14 commits into
python:mainfrom
clin1234:patch-4
ConversationCommits (14)ChecksFiles changed
Closed

gh-149965: promote ptr_wise_atomic_memmove to internal header#151255
clin1234 wants to merge 14 commits into
python:mainfrom
clin1234:patch-4

Conversation

clin1234 commented Jun 10, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

clin1234 and others added 12 commits June 10, 2026 09:55
Update _elementtree.c
2e75ad7
📜🤖 Added by blurb_it.
a3a7eee
Modify critical section assertion in _elementtree.c
ae5d233 
Commented out critical section assertion for ElementObject.
Update Misc/NEWS.d/next/Library/2026-05-17-22-42-54.gh-issue-149965.O…
457fec2 
…FW3ZD.rst

Co-authored-by: Sergey Miryanov <sergey.miryanov@gmail.com>
pythongh-149966: deduplicate ptr_wise_atomic_memmove into a shared in…
a4c9efb 
…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>
Use _Py_ptr_wise_atomic_memmove everywhere in listobject.c
8136bef
Keep _testlimitedcapi/weakref.c in Setup.stdlib.in
1b1e2d0
Cast 1st arg of all calls to _Py_ptr_wise_atomic_memmove to PyObject*
9a55b29
Make test shut up check-c-globals.py
20dac52
Drop const in test_objs
c15572a
Revert previous changes
18b1aac
Delete old blurb
3a3a88d
bedevere-app Bot mentioned this pull request Jun 10, 2026
Open

clin1234 commented Jun 10, 2026

Copy link
Copy Markdown
Contributor Author

@ericsnowcurrently, mind taking a look on helping me on resolving the mutable global variable issue in Modules/_testinternalcapi/test_ptr_wise_memmove.c?

ZeroIntensity requested changes Jun 10, 2026
View reviewed changes

ZeroIntensity left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Comment thread Misc/NEWS.d/next/Core_and_Builtins/2026-06-10-14-01-11.gh-issue-149965.edk2ut.rst Outdated

ZeroIntensity Jun 10, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Internal changes don't need changelog entries; you can remove this.

bedevere-app Bot commented Jun 10, 2026

Copy link
Copy Markdown

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 I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

clin1234 commented Jun 10, 2026

Copy link
Copy Markdown
Contributor Author

@ericsnowcurrently, mind taking a look on helping me on resolving the mutable global variable issue in Modules/_testinternalcapi/test_ptr_wise_memmove.c?

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.

The reason for the generalized implementation is that I found it helpful for replacing certain memmoves in _elementtree.c, and because in the previous PR, someone suggested unit tests for this function: #149966 (comment)

clin1234 commented Jun 10, 2026

Copy link
Copy Markdown
Contributor Author

I have made the requested changes; please review again.

bedevere-app Bot commented Jun 10, 2026

Copy link
Copy Markdown

Thanks for making the requested changes!

@ZeroIntensity: please review the changes made to this pull request.

bedevere-app Bot requested a review from ZeroIntensity June 10, 2026 16:44

ZeroIntensity commented Jun 10, 2026

Copy link
Copy Markdown
Member

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.

ZeroIntensity mentioned this pull request Jun 10, 2026
Open
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

erlend-aasland Awaiting requested review from erlend-aasland erlend-aasland is a code owner
AA-Turner Awaiting requested review from AA-Turner AA-Turner is a code owner
emmatyping Awaiting requested review from emmatyping emmatyping is a code owner
itamaro Awaiting requested review from itamaro itamaro is a code owner
ZeroIntensity Awaiting requested review from ZeroIntensity

Assignees

No one assigned

Labels

awaiting change review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

Back | FazBrowse Home | New Git URL