no-std feature flag.
#141
no-std feature flag.
#141
Conversation
This commits uses the `core` and `alloc` crate instead of std as much as possible, without any changes to the code. We are simply importing from a different location.
This commit changes std::os::unix::io::RawFd into simply c_int, which exists in `core`.
The worst change here is substituting `last_os_error` to just reading `errno` from `libc`. However, since ALSA can only be used in `linux` systems, which will always have `errno` in their `libc`, with the correct semantics, this should be fine.
diwic
left a comment
diwic
left a comment
There was a problem hiding this comment.
Hi and thanks for your work!
I think it looks good for the most part, I have a few questions/thoughts, see review comments. Thanks!
src/direct/pcm.rs
Outdated
| use core::{mem, ptr, fmt, cmp}; | ||
| use crate::error::{Error, Result}; | ||
| use std::os::unix::io::RawFd; | ||
| use core::ffi::c_int; |
There was a problem hiding this comment.
As for RawFd. Would it be possible to declare it somewhere like
#feature(std)
type RawFd = std::os::unix::io::RawFd;
#feature(no-std)
type RawFd = core::ffi::c_int;
...and then use that definition of RawFd everywhere else?
Makes the code more readable I believe.
src/lib.rs
Outdated
| pub mod seq; | ||
| pub use crate::seq::Seq as Seq; | ||
|
|
||
| /// The io module uses the thread_local macro. There is no equivalent of it in `core` or `alloc`, |
There was a problem hiding this comment.
Was this comment left behind? You do compile the io module either way.
| /// | ||
| /// Sometimes alsa-lib writes to stderr, but if you prefer, you can write it here instead. | ||
| /// Should you wish to empty the buffer; just call local_error_handler again and drop the old instance. | ||
| #[cfg(feature = "std")] |
There was a problem hiding this comment.
Do you think we need to write in the comment to this function that it's unavailable in no-std mode or is this obvious when you see it in docs.rs later?
There was a problem hiding this comment.
I thought rustdoc did this automatically, but it turns out it is an unstable feature. I've added a comment as you requested.
| pub type Result<T> = ::std::result::Result<T, Error>; | ||
| pub type Result<T> = ::core::result::Result<T, Error>; | ||
|
|
||
| extern "C" { |
There was a problem hiding this comment.
I think we should have different implementations for std and no-std mode. So that the errno import is only done in no-std mode, keeps the std mode cleaner. Like
#feature(std)
std::io::Error::last_os_error().raw_os_error().unwrap_or_default()
#feature(no-std)
unsafe { errno }
src/direct/pcm.rs
Outdated
| std::println!("{:?}", m); | ||
| let mut i = (0..(m.buffer_size() * 2)).map(|i| | ||
| (((i / 2) as f32 * 2.0 * ::std::f32::consts::PI / 128.0).sin() * 8192.0) as i16); | ||
| (((i / 2) as f32 * 2.0 * ::core::f32::consts::PI / 128.0).sin() * 8192.0) as i16); |
There was a problem hiding this comment.
Is this change needed as the std crate is imported?
There was a problem hiding this comment.
It is not. I just fixed it.
2 participants
Closes #140.
I tried dividing it in multiple commits to make it easier to review.
First two commits should be very straightforward. The first was literally done with with a few
find -exec sedcommands.The third is where we had to change a few things to make them work in both
stdandno-stdenvironments. In particular, we changedlast_os_errorto readingerrnodirectly. My understanding is that those should be equivalent in unix systems, which is all ALSA cares about.The fourth commit adds a check for the
no-stdflag to CI.Going forward, supporting this flag should be relatively easy: use
core::andalloc::instead ofstdin as many places as possible. In particular, this includes usingalloc::vec::Vecandalloc::string::Stringinstead of the containers fromstd(the implementation is actually the same,stdjust re-exports them). If there is ever a place where that cannot be done, if should be notated with#[cfg(not(feature = "no-std"))]. We have to do this in only 3 locations currently: twostd::io::{Read, Write}implementations and one use of athread_local!global.EDIT: one more thing is that you will have to manually declare
extern crate stdin tests functions to use thestdcrate.