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

[WIP] Add no_std + alloc support #1438

Closed
wants to merge 1 commit into from
Closed

Conversation

taiki-e
Copy link
Member

@taiki-e taiki-e commented Jan 28, 2019

This adds the alloc features to futures, futures-core, futures-sink, and futures-util.

This allows to use Box<Future>, alloc::task, many *Ext traits's methods etc in the no_std + alloc environment.

This PR adds alloc-shim crate as an optional dependency. This is only valid if the alloc feature is enabled.
alloc-shim will re-export items of core and alloc in the same layout as std. This allows adding no_std + alloc support without changing existing imports.
Edit: The current alloc-shim has the same layout as the alloc crate.

Some other points:

  • When using the alloc feature, the nightly feature must be used at the same time (see this comment in no_std + alloc feature selection #626).

  • #[cfg(alloc)] is the same as #[cfg(any(feature = "alloc", feature = "std"))], but it is simpler and easier to write (build.rs was added for this).
    Edit: The implementation was changed to use #[cfg(feature = "alloc")] (thanks! @Nemo157).

  • futures::stream::ReuniteError implements std::error::Error only if "std" feature is valid.

  • Fixed an unused_imports warning in no_std build. Edit2: It was split into Fix unused_imports warning on --no-default-features build #1447

Reason to add alloc-shim to dependencies

The layout in the prelude module is different for std and alloc, so alloc::prelude::v1::* can not be used (and alloc::sync does not include atomic module). Also, until now, we've imported everything from std, but from now on we'll need to import them from alloc and core, respectively. Using alloc-shim can avoid these problems.

alloc crate is still unstable, so I think alloc-shim will be unnecessary in the future, but until then it will be possible to support no_std + alloc with less cost by using alloc-shim (also, alloc-shim is designed so that the build will not fail even if atomic module is added to alloc::sync).

Also, I've examined several other ways (1, 2, 3), but I believe this is the simplest and cleanest.

Edit:

Using alloc-shim can avoid the first problem.

Also, this is an implementation that does not use alloc-shim: alloc-no-deps

@taiki-e
Copy link
Member Author

taiki-e commented Jan 28, 2019

Related: #626

cc @cramertj, @Nemo157

@taiki-e
Copy link
Member Author

taiki-e commented Jan 28, 2019

alloc-shim is a very simple crate as follows:

Edit: I deleted the old code. It is now implemented like this.

(The alternative to alloc-shim I think is to create a crate in futures-rs that provide equivalent features.)

@jrobsonchase
Copy link
Contributor

jrobsonchase commented Jan 28, 2019

`pub use core::{str, *};`

Probably don't really need to facade anything from core since it's already available everywhere.

pub use liballoc::{alloc, borrow, fmt, slice, task, *};

Why import all of these specifically and then the glob as well?

    pub mod prelude {
        /// The alloc Prelude
        pub mod v1 {

Is the goal here to imitate the alloc crate or std? There's no alloc::prelude::v1 module.

Overall I really like the idea behind alloc-shim, but I think we need to make up our minds about what its actual goal is. IMO, it should strive to look as close to alloc itself as possible and nothing more.

Edit:

Ah, I glossed over this from your original post:

alloc-shim will re-export items of core and alloc in the same layout as std.

I'm still of the opinion that if it's an alloc shim, it should look like alloc, not std.

Edit 2: Some reasoning behind my preferences:

I feel like when no_std or alloc-only support are goals, it's best to be explicit about where things are coming from and what's actually available in a given context. If it looks like everything is coming from std, it becomes a lot easier to break the alloc build by accidentally importing something from the fake alloc-shim std that doesn't actually exist when you turn off std, and then only finding out about it when CI goes to build the alloc variant. Rewriting the imports to explicitly use the lowest API level needed may introduce more churn now, but could be better for maintainability in the longer term.

@Nemo157
Copy link
Member

Nemo157 commented Jan 28, 2019

One goal that I think this should aim for is minimal changes (and no extra dependencies) once alloc is stabilized. So, while this results in the least churn right now, alloc-no-deps appears to me to be moving closest to that endgoal (although because of the slight differences between std and alloc's API this may still want to use something like extern crate alloc_shim as alloc; to map std -> alloc).


I just thought of an alternative to the build-script based detection, you can have your original plan of std implying alloc, then force the extra nightly feature when not using std with something like

#[cfg(all(feature = "alloc", not(any(feature = "std", feature = "nightly"))))]
compile_error!("The `alloc` feature without `std` requires the `nightly` feature active to explicitly opt-in to unstable features");

Then switch back to using #[cfg(feature = "alloc")] everywhere else.

@taiki-e
Copy link
Member Author

taiki-e commented Jan 29, 2019

I just thought of an alternative to the build-script based detection, you can have your original plan of std implying alloc, then force the extra nightly feature when not using std with something like

It's great, thanks!


Previously I thought that it would be better for us to use alloc like std, but now I think that this is a bad idea (@jrobsonchase, @Nemo157 Thanks for pointing that out). And I rewrote this PR and alloc-shim so that it was almost the same as after alloc crate was stabilized.

@taiki-e taiki-e changed the title Add no_std + alloc support [WIP] Add no_std + alloc support Feb 5, 2019
@taiki-e
Copy link
Member Author

taiki-e commented Feb 5, 2019

When rust-lang/rust#58175 is done, I will update this PR and write a new summary.

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