Skip to content

Add PyFutureCallback type alias for the asyncio Future-resolving callback#1040

Open
Matt711 wants to merge 2 commits into
rapidsai:mainfrom
Matt711:imp/future-resolve-callback
Open

Add PyFutureCallback type alias for the asyncio Future-resolving callback#1040
Matt711 wants to merge 2 commits into
rapidsai:mainfrom
Matt711:imp/future-resolve-callback

Conversation

@Matt711
Copy link
Copy Markdown
Contributor

@Matt711 Matt711 commented May 15, 2026

Because function pointers are confusing to read. And this helps some.

@Matt711 Matt711 requested a review from a team as a code owner May 15, 2026 02:07
@Matt711 Matt711 added non-breaking Introduces a non-breaking change improvement Improves an existing functionality labels May 15, 2026
*/
coro::task<void> cython_libcoro_task_wrapper(
void (*cpp_set_py_future)(void*, const char *),
PyFutureCallback cpp_set_py_future,
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Or maybe AsyncIOFutureCallback?

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.

What about cython_libcoro_task_wrapper_callback to avoid the name clash? Since this is in a .pxd, it will effectively be copy-pasted into all Cython files importing it.

Alternatively, we could wrap the whole thing in a namespace.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

What about cython_libcoro_task_wrapper_callback to avoid the name clash?

Is there a current name clash or are you thinking of protecting against another .pxd using the same name with a different type?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Let's put the whole thing in an anonymous namespace I think.

Although then I would worry about ODR-use violations...

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.

Is there a current name clash or are you thinking of protecting against another .pxd using the same name with a different type?

There is no current name clash. This is mostly about avoiding a future one.

Since this lives under _detail, we can always rename it later without a
deprecation period. Still, I think it is good style to use a unique name because this is
declared in a .pxd and may be pulled into different generated C++ files.

I do not think an anonymous namespace helps here. A cdef extern from * block
is effectively copied into the generated C++ source of each module that imports
it. So if a .pyx file also defines or imports a PyFutureCallback with a
different type, the names can still collide within that generated translation
unit.

That is why I prefer a more specific name like cython_libcoro_task_wrapper_callback.

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

Labels

improvement Improves an existing functionality non-breaking Introduces a non-breaking change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants