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 support (closes #11) #12

Merged
merged 1 commit into from
Aug 6, 2017
Merged

no_std support (closes #11) #12

merged 1 commit into from
Aug 6, 2017

Conversation

tarcieri
Copy link
Contributor

@tarcieri tarcieri commented Aug 5, 2017

Changes references from std:: to core:: where applicable, ensuring the crate builds in no_std environments.

@tarcieri tarcieri changed the title no_std support no_std support (closes #11) Aug 5, 2017
src/hide.rs Outdated
@@ -63,7 +63,7 @@ mod nightly {
}

// When a C compiler is available, a dummy C function can be used.
#[cfg(not(feature = "no_cc"))]
#[cfg(all(not(feature = "no_cc"), feature = "std"))]
mod cc {
use std::os::raw::c_void;
Copy link
Contributor Author

@tarcieri tarcieri Aug 5, 2017

Choose a reason for hiding this comment

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

Wow, using std::os::raw::c_void from no_std is one deep rabbit hole. Some decent background in this thread:

https://internals.rust-lang.org/t/solve-std-os-raw-c-void/3268/16

I guess this is the path forward, but work has not yet begun:

rust-lang/rust#43467

An interim option is to use libc::c_void from the libc crate, but no_std users are presently stuck on nightly anyway for a number of reasons, so simply using the nightly features seems to me like the best solution for no_std users for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On second thought, the std feature I added exists exclusively to support this. It'd be nice to not have a std feature at all.

An interim solution would be to change the no_cc feature to be a default cc feature (features are supposed to be additive anyway), and have an optional dependency on libc pulled in when the cc feature is enabled.

@tarcieri tarcieri force-pushed the no_std branch 4 times, most recently from f012580 to 34c7cd1 Compare August 5, 2017 18:11
@tarcieri
Copy link
Contributor Author

tarcieri commented Aug 5, 2017

Well, I redid this as a breaking change (see f012580 for an attempt to do it mostly backwards compatibly).

I changed the no_cc feature to a cc feature (cargo features should be additive anyway), and used that to gate pulling in the libc, as that seems like the best way to get at c_void for now. It seems this is a bit of a quagmire, and std::os::raw should be deprecated, but there isn't a good replacement now besides the libc crate:

https://internals.rust-lang.org/t/solve-std-os-raw-c-void/3268/16

Apparently this is the path forward:

rust-lang/rust#43467

@tarcieri tarcieri force-pushed the no_std branch 2 times, most recently from 8ac1828 to 4341586 Compare August 5, 2017 18:23
@cesarb
Copy link
Owner

cesarb commented Aug 6, 2017

You missed the documentation comments and the readme, which also reference no_cc.

While void * is more elegant (and core really should have a way to represent it), the C function clear_on_drop_hide is an implementation detail. It would be simple to change its signature to unsigned char *clear_on_drop_hide(unsigned char *ptr), and on the Rust side use u8 instead of c_void (although char * and c_char would be better than unsigned char and u8, but again we would need c_char). That way, you don't need the libc crate, and can use the C function variant even with core (the fallback is not as reliable, best to avoid it if at all possible).

Changes references from std:: to core:: where applicable, ensuring the crate
builds in no_std environments.
@tarcieri
Copy link
Contributor Author

tarcieri commented Aug 6, 2017

Okay, I changed clear_on_drop_hide's signature to use unsigned char *ptr and changed the Cargo feature back to no_cc (I still think a positive cc feature would make more sense, but that's a topic for a separate PR if it can be avoided in this one)

@cesarb cesarb merged commit fae4f56 into cesarb:master Aug 6, 2017
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