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

Make std feature to depend on alloc feature #508

Merged
merged 5 commits into from
May 22, 2020
Merged

Conversation

taiki-e
Copy link
Member

@taiki-e taiki-e commented May 20, 2020

alloc crate is available on 1.36, see #368 (comment) for why it was previously separated.

This is a breaking change proposed in #503.

@ghost
Copy link

ghost commented May 20, 2020

There's an unused #[macro_use].

@taiki-e
Copy link
Member Author

taiki-e commented May 20, 2020

CI passed.

@taiki-e taiki-e requested a review from a user May 20, 2020 10:09
@taiki-e taiki-e changed the title Change std feature to depend on alloc feature Make std feature to depend on alloc feature May 20, 2020
Currently, crossbeam-epoch is empty crate if alloc feature disabled.
@taiki-e
Copy link
Member Author

taiki-e commented May 20, 2020

If we add a public API that does not depend on alloc to crossbeam-epoch, in order to use it in no-std environment where alloc crate cannot be used, it is necessary to separate APIs that require alloc crate by a feature gate.

However, this is a breaking change because adding this feature will prevent crate, which currently uses crossbeam-epoch with default-feature = false, from accessing APIs that require alloc crate.

As a similar issue, adding no-std support to crossbeam-channel and crossbeam-deque, which currently do not have feature flags and depend on std, is actually a breaking change for crates using these crate with default-feature = false. (Actually, this is why futures-io has std feature).

So, unless it's absolutely impossible for these crates to add support no-std, I tend to prefer to add std feature to these crates...
@stjepang @jeehoonkang Any thoughts about this?

@jeehoonkang
Copy link
Contributor

  • Maybe in the future we want crossbeam-deque to support no-std with a custom allocator. So I'd say it's plausible to support no-std in crossbeam-deque, e.g.

  • I think it's relatively easy to break backward compatibility for the time being, as we didn't reach 1.0 yet. I prefer to have a cleaner feature configurations regarding std. @taiki-e what do you think is the best practice? Is the std feature the clean and widely used measure for this?

@ghost
Copy link

ghost commented May 20, 2020

So, unless it's absolutely impossible for these crates to add support no-std, I tend to prefer to add std feature to these crates...

Sounds good to me, let's do it.

@taiki-e
Copy link
Member Author

taiki-e commented May 20, 2020

@jeehoonkang

what do you think is the best practice?

There is a discussion on this in rust-lang/api-guidelines#189, but as far as I know, it's the only way.

Also, it may be nice to prevent users from accidental compilation without std:

#[cfg(not(feature = "std"))]
compile_error!("`std` feature is currently required to build this crate");

Is the std feature the clean and widely used measure for this?

For example regex is one of the 1.0+ crates that do this, but I'm not sure how many crates actually use this way.

@taiki-e
Copy link
Member Author

taiki-e commented May 20, 2020

Also, it may be nice to prevent users from accidental compilation without std:

Ah, this approach doesn't work with crossbeam, as we depend on these crates via nightly feature.

nightly = ["crossbeam-epoch/nightly", "crossbeam-utils/nightly", "crossbeam-queue/nightly"]

@taiki-e
Copy link
Member Author

taiki-e commented May 20, 2020

Also, I noticed that crossbeam-utils' alloc feature isn't really doing anything, so I removed it.

@taiki-e
Copy link
Member Author

taiki-e commented May 20, 2020

Added document to feature flags (86a6aa6):

# Enable to use APIs that require `std`.
# This is enabled by default.
std = ["alloc"]

# Enable to use APIs that require `alloc`.
# This is enabled by default and also enabled if the `std` feature is enabled.
alloc = []

# Enable to use of unstable functionality.
# This is disabled by default and requires recent nightly compiler.
# Note that this is outside of the normal semver guarantees and minor versions
# of crossbeam may make breaking changes to them at any time.
nightly = []

@jeehoonkang
Copy link
Contributor

I'm happy with this PR. @stjepang do you have more to discuss? If not, I'll merge it 2 days later.

Thanks!

@jeehoonkang
Copy link
Contributor

bors r+

@bors bors bot merged commit c5df6cf into crossbeam-rs:master May 22, 2020
@taiki-e taiki-e deleted the alloc branch May 22, 2020 15:17
exrook pushed a commit to exrook/crossbeam that referenced this pull request Oct 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants