Skip to content

No std ThreadId#6056

Open
Person-93 wants to merge 4 commits into
PyO3:mainfrom
Person-93:no_std_thread_id
Open

No std ThreadId#6056
Person-93 wants to merge 4 commits into
PyO3:mainfrom
Person-93:no_std_thread_id

Conversation

@Person-93
Copy link
Copy Markdown
Contributor

Re-implement ThreadId using PyThread_get_thread_ident. This makes is usable in no_std context.

Copy link
Copy Markdown
Contributor

@clin1234 clin1234 left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Copy Markdown
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

Thanks, one comment on the FFI definition (see below).

Do you expect that the test suite (which does a lot of thread spawning) will need to change?

Comment thread pyo3-ffi/src/pythread.rs
use core::num::NonZero;

extern_libpython! {
pub fn PyThread_get_thread_ident() -> NonZero<u64>;
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.

There is also PyThread_get_thread_native_id. Do you know how these might differ?

I see that PyThread_get_thread_native_id is only available on some platforms, it might be challenging to define that availability with cfgs. (We could probably get it from the interpreter sysconfig when available, and make assumptions for "default" stable ABI builds.)

Also, we should use c_ulong as the return type here, to match the header.

The niche is interesting but we haven't made those part of the FFI definitions previously (e.g. many pointers could realistically be NonNull<T> rather than *mut T).

Suggested change
pub fn PyThread_get_thread_ident() -> NonZero<u64>;
pub fn PyThread_get_thread_ident() -> c_ulong;

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants