-
Notifications
You must be signed in to change notification settings - Fork 292
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
alloc support (for no_std environments) #256
Conversation
Feature gates anything dependent on `std` on the eponymous cargo feature, and changes references to libcore features from `std` to `core`
@@ -63,6 +67,7 @@ impl<'a> IntoBuf for &'a [u8] { | |||
} | |||
} | |||
|
|||
#[cfg(feature = "std")] | |||
impl<'a> IntoBuf for &'a mut [u8] { | |||
type Buf = io::Cursor<&'a mut [u8]>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To really support alloc
properly I think these impl
s (both this one and the one below for &'a str
need to change to not use an io::Cursor
), however as Buf
is an associated type, I imagine it's considered part of the public API and changes to the underlying Buf
type would be considered breaking. As a breaking change I think ideally we could fix this by eliminating all usages of io::Cursor
from the crate (which, I believe, was @carllerche's plan for v0.5.x anyway).
However I'd like to stress that even without these impls, bytes
+ alloc
is still useful, because downstream no_std
+ alloc
consumers of bytes
can define their own buffer type (possibly just vendoring io::Cursor
from libstd
and using the existing std
-dependent impls) and then impl FromBuf
and impl IntoBuf
for their own type. This would be my planned approach for now if this PR were to be merged.
pub use alloc::vec::Vec; | ||
|
||
#[cfg(feature = "std")] | ||
pub use std::prelude::v1::*; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks a little bit weird because it pulls these types in from different places depending on the precise set of features enabled, so it doesn't look purely additive, however std
just re-exports the alloc
types as a facade, so it winds up being purely additive.
Ideally we could always pull the types in from alloc
, and completely get rid of this module, however that would depend on alloc stabilization and would therefore be a breaking change in the minimum supported Rust version.
Gates all allocator-dependent features on the 'alloc' cargo feature, allowing use in no_std environments which have a heap and defined allocator, but not full support for 'std'.
Thanks. At this point, focus is shifting towards the next breaking release. I’ll aim to get this included. |
Please take a look at #269, my design proposal to abstract away the allocator. |
@mzabaluev Looks good to me! I'd support that approach over what's in this PR |
Obsoleted by #281 |
This PR revives #153 and includes #255. I'm reopening it because the
alloc
crate is now stable, which should hopefully address the previous concerns about #153 beingnightly
-dependent.It gates all allocator-dependent features on the
alloc
cargo feature, enabling use of much of the functionality inno_std
environments which have a heap and defined allocator, but not full support for 'std'.Unlike previous attempts, this PR runs the parts of the test suite which do not require
std
as part of CI, ensuring thatalloc
support is properly tested. For now this requiresnightly
, but whenalloc
stabilization lands in thestable
channel, it should be possible to always testalloc
support in all environments.