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] crossbeam-epoch: expose loom support via feature flag #638

Closed
wants to merge 1 commit into from
Closed

[WIP] crossbeam-epoch: expose loom support via feature flag #638

wants to merge 1 commit into from

Conversation

tobz
Copy link
Contributor

@tobz tobz commented Jan 3, 2021

This PR exposes the loom_crossbeam feature flag to enable internal loom usage, rather than relying on cfg-based settings.

The usage of --cfg flags is cumbersome as it forces recompilation of all crates given that RUSTFLAGS has changed.

@tobz tobz changed the title crossbeam-epoch: expose loom feature flag {WIPP] crossbeam-epoch: expose loom support via feature flag Jan 3, 2021
@tobz tobz changed the title {WIPP] crossbeam-epoch: expose loom support via feature flag [WIP] crossbeam-epoch: expose loom support via feature flag Jan 3, 2021
@tobz tobz marked this pull request as draft January 3, 2021 22:20
@taiki-e
Copy link
Member

taiki-e commented Jan 4, 2021

I understand the motivation, but the cargo feature needs to be additive and I don't think the loom feature meets that requirement.

@tobz
Copy link
Contributor Author

tobz commented Jan 4, 2021

I'm happy to stick with the --cfg based approach if the feature approach isn't workable, but can you explain a bit more?

From my perspective, the feature flag would enable pulling in loom itself as well as flipping the codepaths from std types to loom types. Is there something else that I'm missing?

@taiki-e
Copy link
Member

taiki-e commented Jan 4, 2021

can you explain a bit more?

Feature flags are silently shared across the dependencies graph.
For example, if I create a crate for the test that always depends on the loom feature and I run cargo test with --all/--workspace, the loom feature will be enabled for all crates. This will break tests on all other crates.
See also rust-lang/cargo#4328, rust-lang/rfcs#1841, rust-lang-nursery/lazy-static.rs#150, serde-rs/serde#918, https://rust-lang.github.io/api-guidelines/naming.html#feature-names-are-free-of-placeholder-words-c-feature

the feature flag would enable pulling in loom itself as well as flipping the codepaths from std types to loom types.

This also applies when using cfg, right?

@tobz
Copy link
Contributor Author

tobz commented Jan 4, 2021

Feature flags are silently shared across the dependencies graph.
For example, if I create a crate for the test that always depends on the loom feature and I run cargo test with --all/--workspace, the loom feature will be enabled for all crates. This will break tests on all other crates.
See also rust-lang/cargo#4328, rust-lang/rfcs#1841, rust-lang-nursery/lazy-static.rs#150, serde-rs/serde#918, https://rust-lang.github.io/api-guidelines/print.html#feature-names-are-free-of-placeholder-words-c-feature

I see what you mean now, and this makes sense. Appreciate the links. 👍🏻

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