-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Move futex locks to sys_common
#104329
Move futex locks to sys_common
#104329
Conversation
Hey! It looks like you've submitted a new PR for the library teams! If this PR contains changes to any Examples of
|
} else { | ||
pub(crate) use crate::sys::locks::{Mutex, RwLock, Condvar}; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This probably isn't the right place either because sys_common shouldn't contain platform cfg() stuff even though tidy currently doesn't complain about it either.
I was going to suggest moving it to sys::common
instead but its mod.rs does say
rust/library/std/src/sys/common/mod.rs
Lines 1 to 5 in cd12888
// This module contains code that is shared between all platforms, mostly utility or fallback code. | |
// This explicitly does not include code that is shared between only a few platforms, | |
// such as when reusing an implementation from `unix` or `unsupported`. | |
// In those cases the desired code should be included directly using the #[path] attribute, | |
// not moved to this module. |
So the #[path] seems to be the approved way already.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this concerns other code like the thread parker and one-time initialization as well, I suggest we discuss this in #84187 (the tracking issue for that guideline).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't want to devalue the contributions of anyone who has worked on libstd, but this mostly seems like it was one person's idea 2 years ago about what would be a good organization of stuff, tbh. Since then we've sprouted a lot more small niche mostly-tier-3 targets that are mostly just repeats of Linux or other POSIX platforms, yet with enough annoying variations that they induce quite a lot of configuration around that. I think we should revisit those guidelines heavily.
☔ The latest upstream changes (presumably #106228) made this pull request unmergeable. Please resolve the merge conflicts. |
This PR is superseded by #117276, closing. |
These implementations are also used on WASM and Hermit, so it does not really make sense to keep them in
sys::unix
, especially considering that the current path-based imports have already lead to breakage (#99800). Moving them tosys_common
mirrors the organization of the thread parking implementations, and provides a good place for future generic lock implementations (see #93740) to go.r? @m-ou-se