Skip to content

Fix documentation for gcvisitobjects_t #132433

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Apr 12, 2025
Merged

Fix documentation for gcvisitobjects_t #132433

merged 2 commits into from
Apr 12, 2025

Conversation

da-woods
Copy link
Contributor

@da-woods da-woods commented Apr 12, 2025 •
edited by github-actions bot
Loading

Documented behaviour is the exact opposite of the real behaviour.

See comment in the code

cpython/Include/cpython/objimpl.h

Lines 92 to 94 in ad3bbe8

* to the supplied arg. Returning 0 from the callback ends iteration, returning
* 1 allows iteration to continue. Returning any other value may result in
* undefined behaviour.

and code

cpython/Python/gc.c

Lines 2403 to 2406 in ad3bbe8

int res = callback(op, arg);
Py_DECREF(op);
if (!res) {
return -1;

📚 Documentation preview 📚: https://cpython-previews--132433.org.readthedocs.build/

@bedevere-app bedevere-app bot added awaiting review docs Documentation in the Doc dir skip news labels Apr 12, 2025
@github-project-automation github-project-automation bot moved this to Todo in Docs PRs Apr 12, 2025
@picnixz
Copy link
Member

picnixz commented Apr 12, 2025

Returning 0 from the callback ends iteration

This is correct though? because if (!0) would be true in this case.

@picnixz picnixz added the pending The issue will be closed if no feedback is provided label Apr 12, 2025
@da-woods
Copy link
Contributor Author

Returning 0 from the callback ends iteration

This is correct though? because if (!0) would be true in this case.

I think that's consistent - the code comment and the code say the same thing - that returning 0 ends the iteration. The rst docs say the opposite thing (which this PR aims to fix)

@picnixz
Copy link
Member

picnixz commented Apr 12, 2025

Ah! I thought that the error was in the C comment, not the rst comment (and since the text is almost the same I didn't see I was looking at the rst). My bad!

@picnixz picnixz removed the pending The issue will be closed if no feedback is provided label Apr 12, 2025
@picnixz picnixz added the needs backport to 3.13 bugs and security fixes label Apr 12, 2025
@picnixz
Copy link
Member

picnixz commented Apr 12, 2025

@hugovk This kind of doc update could be backported in 3.12 because otherwise users will have a pretty important API misuse. Can this be considered backportable?

@hugovk
Copy link
Member

hugovk commented Apr 12, 2025

We normally only backport security fixes to security branches. I expect most people read the docs at /3/ and not /3.12/, even if they're using 3.12.

But this is a small, safe change, so you can ask 3.12 RM Thomas if you'd like an exception.

@picnixz picnixz merged commit 1e5798e into python:main Apr 12, 2025
24 checks passed
@miss-islington-app
Copy link

Thanks @da-woods for the PR, and @picnixz for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖

@github-project-automation github-project-automation bot moved this from Todo to Done in Docs PRs Apr 12, 2025
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Apr 12, 2025
@da-woods @miss-islington
Docs: Fix specifications of gcvisitobjects_t (pythonGH-132433)
5038d25 
`gcvisitobjects_t` callbacks should return 1 for the iteration to continue instead of 0.
(cherry picked from commit 1e5798e)

Co-authored-by: da-woods <dw-git@d-woods.co.uk>
@bedevere-app
Copy link

bedevere-app bot commented Apr 12, 2025

GH-132441 is a backport of this pull request to the 3.13 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label Apr 12, 2025
@picnixz
Copy link
Member

picnixz commented Apr 12, 2025

@Yhg1s Would you be willing to consider this change as a backportable one? (note that the typedef was added in 3.12)

picnixz pushed a commit that referenced this pull request Apr 12, 2025
@miss-islington @da-woods
[3.13] Docs: Fix specifications of gcvisitobjects_t (GH-132433) (#1…
1884a3b 
…32441)

Docs: Fix specifications of `gcvisitobjects_t` (GH-132433)

`gcvisitobjects_t` callbacks should return 1 for the iteration to continue instead of 0.
(cherry picked from commit 1e5798e)

Co-authored-by: da-woods <dw-git@d-woods.co.uk>
@da-woods da-woods deleted the da-woods-patch-1 branch April 12, 2025 16:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@picnixz picnixz picnixz approved these changes

Assignees
No one assigned
Labels
docs Documentation in the Doc dir skip issue skip news
Projects
Docs PRs
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@da-woods @picnixz @hugovk