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 (extends 2018 edition) #18

Closed
wants to merge 1 commit into from
Closed

No std (extends 2018 edition) #18

wants to merge 1 commit into from

Conversation

WildCryptoFox
Copy link

@WildCryptoFox WildCryptoFox commented Nov 24, 2018

Messing with the imports for 2015 edition is more annoying for no_std and my attempts failed. Sure it can be done, but it isn't worth it as there is no reason against using the 2018 edition.

Blocking on #16.

@WildCryptoFox
Copy link
Author

WildCryptoFox commented Nov 24, 2018

Okay. On 2015 edition the way to do no_std is to add a mod std and I was trying mod core for std. On 2018 edition, core is always included, so even if it is in std-mode, it'll just work.

The alloc import isn't 2018 idiomatic because it does extern crate, but unfortunately this is necessary for alloc right now for #![no_std]. It should be implicitly included by its usage.

If you don't want the 2018 edition yet, I can take another attempt at 2015 no_std, but that effort would be undone when you accept 2018.

@@ -2,6 +2,7 @@
#![doc(html_root_url = "https://docs.rs/slotmap/0.3.0")]
#![crate_name = "slotmap"]
#![cfg_attr(feature = "unstable", feature(untagged_unions))]
#![cfg_attr(not(feature = "std"), feature(alloc))]
Copy link
Author

Choose a reason for hiding this comment

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

BTW: I had no_std last night, but that wasn't cargo idiomatic as cargo features are additive, so std is more correct (and common).

pub mod sparse_secondary;
pub use sparse_secondary::SparseSecondaryMap;
#[cfg(feature = "std")]
pub use crate::sparse_secondary::SparseSecondaryMap;
Copy link
Author

Choose a reason for hiding this comment

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

HashMap currently requires std because it assumes the default RandomState, which relies on OSRng. rust-lang/rust#56192

@WildCryptoFox
Copy link
Author

(Not 100% sure git is smart enough to merge this without duplicating the SparseSecondaryMap changes... Will update this after that PR is accepted.)

@orlp
Copy link
Owner

orlp commented Nov 24, 2018

Since we support serde I think things are a bit more complicated than usual: https://serde.rs/no-std.html.

I don't know exactly how this would end up working. In particular the last section about alloc and removing support for Vec<T> otherwise is troubling.

We also need to replace serde-json with serde-json-core in our dev dependencies or otherwise we're never testing properly without std.

@WildCryptoFox
Copy link
Author

WildCryptoFox commented Nov 24, 2018

@orlp Oh. Quick fix.

Edit: turns out both default_features and default-features are valid and commonly used but the serde docs promote -. I'll use that.

@WildCryptoFox
Copy link
Author

@orlp Maybe unstable = ["serde/alloc"] ?

@WildCryptoFox
Copy link
Author

@orlp Regarding no-std tests. We aren't really testing no-std tests anyway. The testing infrastructure for no-std is (1) does it build? and (2) does it work under no-std environments? I might tell you the latter soon as I'm working on https://robigalia.org but I'm not ready to test my code using slotmap on that yet.

@WildCryptoFox
Copy link
Author

@orlp Just did a quick pass over serde code. alloc is only respected when not(std), so this is safe.

@WildCryptoFox
Copy link
Author

Ugh. Problem. unstable = ["serde/alloc"] means that serde is enabled...

@orlp
Copy link
Owner

orlp commented Nov 24, 2018

Aren't we missing #![no_std] when not(std) in the crate root?

Ugh. Problem. unstable = ["serde/alloc"] means that serde is enabled...

I was worried about that too... Also not(std) does not imply unstable. I want unstable to only be for unstable additional features within slotmap.

@WildCryptoFox
Copy link
Author

@orlp Wow. Yes... Seems that with all the rebasing and reimplementing ... I missed a few things. o.0

Fixed....

@orlp
Copy link
Owner

orlp commented Nov 24, 2018

It seems that there is no solution: https://www.reddit.com/r/rust/comments/78pn60/enabling_feature_of_optional_dependency/.

Perhaps no std support is just a bit too infantile in Rust to start supporting it.

@WildCryptoFox
Copy link
Author

cargo check --no-default-features --features serde,serde-alloc,unstable

cargo test should not work because the tests assume std.

@WildCryptoFox
Copy link
Author

Yikes.

@WildCryptoFox
Copy link
Author

WildCryptoFox commented Nov 24, 2018

@orlp Fixed #N!

@WildCryptoFox
Copy link
Author

@orlp Including a compile time lint. ;)

@WildCryptoFox
Copy link
Author

Use feature serde or serde-alloc for no-std; use serde-std for std. Using just serde on std will complain with that compile_error!.

@orlp
Copy link
Owner

orlp commented Nov 24, 2018

I don't like that solution because now regular users need to specify --serde-std instead of --serde. Would the following work?

[features]
default = ["std"]
serde = ["serde/std"]
serde-no-std = ["serde/alloc"]
std = []
unstable = []

[dependencies]
serde = { version = "1.0", optional = true, default-features = false, features = ["derive"] }

With an error if serde is used together with not(std)?

@WildCryptoFox
Copy link
Author

@orlp Not quite. You can't have a feature and dependency by the same name. But you could rename the serde dependency internally. serde-x = { package = "serde", version = "1.0", optional = true, .. }

@WildCryptoFox
Copy link
Author

@orlp Also, not all no-std users will want alloc.

@orlp
Copy link
Owner

orlp commented Nov 24, 2018

If a no-std user doesn't want alloc they can't use slotmap...

@WildCryptoFox
Copy link
Author

But I think it is safe to say that slotmap does depend on alloc as it doesn't at least currently use an array or GenericArray anyway...

@WildCryptoFox
Copy link
Author

@orlp For now we assume alloc. But I might send in another time, a PR for a GenericArray backend for non-alloc users.

@orlp
Copy link
Owner

orlp commented Nov 24, 2018

However, my solution then runs in the opposite issue, in that it relies on foo/bar enabling optional depency foo, which in some Github issues I've read they're thinking of changing.

Honestly, all of this is just too big of a headache right now. I think I'd rather wait a couple more months to see what has changed in support of no-std before putting it in slotmap.

@WildCryptoFox
Copy link
Author

@orlp Okay. Sorry to hear that. I can wait to merge this in. I know I'll need it eventually, but I don't use serde and will be fine with my fork. :)

@orlp
Copy link
Owner

orlp commented Nov 24, 2018

Thanks for your contributions regardless. It's not your fault no-std is kind of a mess right now :)

@WildCryptoFox
Copy link
Author

@orlp
Copy link
Owner

orlp commented Jan 1, 2021

With all the changes in no_std Rust and in my code base, it was faster to do this myself from scratch. That said, thanks for the effort and this will be implemented in the upcoming version of slotmap.

@orlp orlp closed this Jan 1, 2021
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.

2 participants