Skip to content
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

gh-127124: Add opaque pointer to context watcher callback #127140

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

rhansen
Copy link
Contributor

@rhansen rhansen 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 opaque pointer.

  • 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(void *arg, PyContextEvent event, 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/

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 opaque pointer.
  * 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(void *arg, PyContextEvent event, PyObject *obj)
        {
            ...
        }
Copy link
Member

@ZeroIntensity ZeroIntensity 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)

Sign up for free to 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