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

Add StableDeref impls for no_std types, including alloc and spin types #2

Merged
merged 2 commits into from Jun 9, 2018

Conversation

kevinaboos
Copy link
Contributor

As a no_std Rustacean, I needed this for my own purposes. But I figured it'd be pretty useful for other people too, especially if they're using owning_ref in their projects.

Happy to make any changes you'd prefer. One thing is that I feel the way you set the default-features is a bit confusing and can lead to weird combinations of features, but I left it as is because I didn't want to break other crates using it.

The new features are used like this:
cargo build --no-default-features --features alloc
cargo build --no-default-features --features spin

@Storyyeller
Copy link
Owner

So just to clarify, alloc is a built in nightly-only crate, while spin is a third party crate, right? I think in the case of spin, it would make more sense to ask the author of spin to add the impls in that crate.

@kevinaboos
Copy link
Contributor Author

That's correct. Regarding spin, many other crates have dependencies on that and treat it as always available (just like std's Mutex types), and even have features that allow you to use spin in place of standard Mutexes, such as lazy_static. So it wouldn't be a stretch to include it in this crate because its often treated as a built-in type.

You'd either be including an optional dependency on spin in this crate, or including an optional dependency on this crate in spin. I'm not really sure what's more idiomatic...

@Storyyeller
Copy link
Owner

Seeing as this crate is literally just a trait definition, I think it makes sense to avoid dependencies. There's no reason why spin can't include an optional dependency on it.

@kevinaboos
Copy link
Contributor Author

Works for me, I'll remove the spin-related parts.

@philipc
Copy link
Contributor

philipc commented Jun 7, 2018

@kevinaboos I have need of the alloc support. Will you be able to update this PR?

@kevinaboos
Copy link
Contributor Author

@philipc ah, I completely forgot about this, got swamped at work. I'll update the PR hopefully within a day or two at the most.

src/lib.rs Outdated
@@ -14,11 +14,19 @@ It is intended to be used by crates such as [owning_ref](https://crates.io/crate
no_std support can be enabled by disabling default features (specifically "std"). In this case, the trait will not be implemented for the std types mentioned above, but you can still use it for your own types.
*/

#![feature(alloc)]
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this need to be #![cfg_attr(feature = "alloc", feature(alloc))]?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's not technically required, but it's more correct that way. Changed, thanks!

@est31 est31 mentioned this pull request Jun 7, 2018
14 tasks
@kevinaboos
Copy link
Contributor Author

Okay, I think it's ready for merging now, @Storyyeller.

@Storyyeller
Copy link
Owner

Ok, I'll try to get to it over the weekend.

@Storyyeller Storyyeller merged commit 94df386 into Storyyeller:master Jun 9, 2018
Storyyeller pushed a commit that referenced this pull request Jun 9, 2018
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