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

no_std #56

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

no_std #56

wants to merge 2 commits into from

Conversation

mio-19
Copy link

@mio-19 mio-19 commented Jun 17, 2021

No description provided.

@mio-19 mio-19 marked this pull request as ready for review June 18, 2021 00:30
@mio-19 mio-19 changed the title (wip) no_std - can not find thread_local for no_std no_std Jun 18, 2021
@vorner
Copy link
Owner

vorner commented Jun 18, 2021

Hello

Maybe I don't fully understand the intention here, but when I was thinking about no-std support, I concluded that it's not actually useful for this crate as it is primarily a thread-synchronization primitive and threads are in std.

Is there a particular use case you have in mind, or is this just an „exercise“?

@mio-19
Copy link
Author

mio-19 commented Jun 19, 2021

Maybe I don't fully understand the intention here, but when I was thinking about no-std support, I concluded that it's not actually useful for this crate as it is primarily a thread-synchronization primitive and threads are in std.

Arc is in alloc and atomic is in core, which is a thread-synchronization primitive.

Is there a particular use case you have in mind, or is this just an „exercise“?

A particular use case is linux kernel module.

@vorner
Copy link
Owner

vorner commented Jun 19, 2021

A particular use case is linux kernel module.

That sounds like that'll be worth the extra maintenance burden, then. I'll find some free time to go over it, hopefully soon ‒ thought that might be few days, I'm a bit busy right now.

Copy link
Owner

@vorner vorner left a comment

Choose a reason for hiding this comment

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

Sorry for taking this long to get around to it. Anyway:

I think the motivation and the general direction is good, but there's quite some amount of little things that need attending to first. Do you want to take them, or should I put them to my TODO list (I kind of like the idea of being conditionally no-std, but I don't know when I could get around to it).

First of all, how lock-free is the library for thread-local storage and initialization? I think it would be OK for it to have some locks on the first access in each thread, or on thread creation. But it would kind of beat the whole purpose of arc-swap if there was a lock in each access. Did you have a chance to study the documentation or implementation?

Many combinations of features & rust versions don't compile even though they did previously. In particular:
• Without enabling the new feature, all previous features need to compile as previously. Specifically, with no features enabled, it must be possible to compile even with 1.31.0.
• I believe this new feature doesn't have to be incompatible with the weak feature.
• I also believe the new feature should be compatible with stable rust, not work only on nightly.

When it comes to the feature, I'm not sure about the name. It's a bit weird when features are „negative“, but they accumulate through dependencies. Also, other features use - for word separator, not _ in this crate. It would be nice to keep that consistent.

The spin-loop feature on that dependency is a must-have on windows, and kind of useless elsewhere. Is it possible to auto-enable it on windows instead of the caller having to somehow manually detect it and enable it or not based on that?

And please adhere to rustfmt 😇

F: FnOnce(&LocalNode) -> R,
{
THREAD_HEAD.try_with(f)
}
Copy link
Owner

Choose a reason for hiding this comment

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

Would it be better to have a whole (inline) module under that #[cfg(...)] instead of repeating it again and again?

Cargo.toml Outdated
@@ -23,8 +23,12 @@ weak = []
internal-test-strategies = []
# Possibly some strategies we are experimenting with. Currently empty. No stability guarantees are included about them.
experimental-strategies = []
# no_std doesn't have RwLock
no_std = ["parking_lot"]

Choose a reason for hiding this comment

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

The cargo recommendation for features is that they be additive, not subtractive: https://doc.rust-lang.org/cargo/reference/features.html#feature-unification

Specifically this would imply a std= feature, make the default features include std; though sadly cargo doesn't support managing the dependency properly : rust-lang/cargo#1839.

Perhaps a comment referencing that issue?

@vorner
Copy link
Owner

vorner commented Oct 7, 2021

Hmm, this seems to have fell asleep a bit. Anyone else wants to take it from here and get it over the finish line? I could try taking it upon myself, but it's not exactly on the top of my priority list right now 😈

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

3 participants