Skip to content

Conversation

@rhansen
Copy link
Contributor

@rhansenrhansen commented Nov 22, 2024

This enables users to associate state with the callback without relying on globals.

Also:

  • Refactor the tests for improved readability and extensibility, and to cover the new state object.

  • Drop the pointer from the PyContext_WatchCallback typedef. This de-obfuscates the fact that pointers are involved, and makes it possible to forward-declare functions to improve readability:

    staticPyContext_WatchCallbackmy_callback; intmy_callback(PyObject*cbarg, PyContextEventevent, PyObject*obj){... }

This will conflict with #124741; if one is merged I'll rebase the other.


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

Copy link
Member

@ZeroIntensityZeroIntensity left a comment

Choose a reason for hiding this comment

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

Needs a NEWS entry (even though it's a change to a new feature, we want to show that we changed it in the a3 changelog)

@rhansenrhansenforce-pushed the gh-127124-context-watcher-opaque-state branch from f6ffc28 to 3130a2fCompareNovember 24, 2024 08:37
@rhansenrhansen changed the title gh-127124: Add opaque pointer to context watcher callbackgh-127124: Pass optional state to context watcher callbackNov 24, 2024
Copy link
ContributorAuthor

@rhansenrhansen left a comment

Choose a reason for hiding this comment

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

I'll open a separate PR for the callable idea I mentioned in #127124 (comment) so that we can compare the two and decide which we like better.

Edit: Done; see #127247.

PyContext_WatchCallbackcontext_watchers[CONTEXT_MAX_WATCHERS];
struct{
PyContext_WatchCallback*callback;
PyObject*arg;
Copy link
ContributorAuthor

@rhansenrhansenNov 24, 2024

Choose a reason for hiding this comment

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

Does this need a Py_VISIT somewhere? I didn't see any for the other fields in this struct, but maybe the GC does something special with this struct?

This enables users to associate state with the callback without relying on globals. Also: * Refactor the tests for improved readability and extensibility, and to cover the new state object. * Drop the pointer from the `PyContext_WatchCallback` typedef. This de-obfuscates the fact that pointers are involved, and makes it possible to forward-declare functions to improve readability: static PyContext_WatchCallback my_callback; int my_callback(PyObject *cbarg, PyContextEvent event, PyObject *obj){... }
@rhansenrhansenforce-pushed the gh-127124-context-watcher-opaque-state branch from 3130a2f to c9e95d4CompareNovember 24, 2024 09:16
@ZeroIntensity
Copy link
Member

FWIW, no need to force push changes--it makes reviewing harder. It's fine to just commit normally, and then everything gets squashed.

@rhansen
Copy link
ContributorAuthor

Force pushing is the only way to update the commit message.

@ZeroIntensity
Copy link
Member

Yes, but it makes reviewing more difficult. FWIW, nobody really cares about the commit messages in the PR itself, the end message that gets squashed is really what matters. (There's a section in the devguide about this, IIRC.)

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@rhansen@ZeroIntensity