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

Use std::thread::ThreadId instead of custom thread id #27

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Nilstrieb
Copy link
Contributor

std guarantees them to be unique, allowing us to use them here instead of having to roll our own thread ids.

`std` guarantees them to be unique, allowing us to use them here instead
of having to roll our own thread ids.
@mitsuhiko
Copy link
Owner

This undoes the niche optimization: #18

@mitsuhiko mitsuhiko closed this Oct 17, 2022
@Nilstrieb
Copy link
Contributor Author

ThreadId uses a NonZeroU64 as well, keeping the niche

@Nilstrieb
Copy link
Contributor Author

It does have the disadvantage of being a little bigger on 32 bit though.

@mitsuhiko
Copy link
Owner

Oh I missed that. I will reopen this for now but I'm not sure yet what the advantage of reusing the threadid from the stdlib is.

@mitsuhiko mitsuhiko reopened this Oct 17, 2022
@Nilstrieb
Copy link
Contributor Author

It makes the code a little simpler and avoids the need for having a custom thread_id module. But since it's a pretty simple module there isn't a huge benefit.

@Nilstrieb
Copy link
Contributor Author

Do you think we should do this? I'd resolve the conflicts then. If not, I can close it

@mitsuhiko
Copy link
Owner

I’m probably in favor of not applying this. It makes the type bigger and I’m not convinced (yet at least) that using the thread ID from the stdlib is necessary. No need to resolve the conflicts but I will leave this open in case my opinion on this changes / gets changed :)

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.

None yet

2 participants