-
Notifications
You must be signed in to change notification settings - Fork 278
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 no_std support, by adding an std
feature
#281
Conversation
Hi -- I'm interested in getting My motivation for doing this is that we want to use I think it's pretty unlikely that all of LMK if you are interested in a patch like this! Thanks |
@tarcieri My PR is very simple -- #269 seems orthogonal and way beyond my goals. there's no reason we can't land this and then that one later.
that seems totally unnecessary. #153 and #135 are both from 2 years ago. Is there anything from these threads in particular that you want to draw attention to? They look moot to me. |
I pointed it out to note that there are other open PRs for this same general issue, for the ample historical discussion if nothing else. #269 describes solutions which can work for heapless re: a crate-local prelude
It’s necessary for compatibility with Rust versions prior to 1.35. It’s certainly much cleaner to support 1.35+, and I’m not sure what MSRV plans are for bytes 0.5. |
@tarcieri I see -- I had mistakenly thought that |
Perhaps @carllerche can tell us what he's thinking re: |
MSRV of Bytes 0.5 is 1.36. Lines 15 to 23 in 234d814
|
@cbeck88 Could you please resolve current conflicts? Master branch targets 2018 so it is perfectly fine as PR. I'm still not sure what sort of no_std support we should have, but I believe support for global allocator would be good first step |
I was really hopeful initially that this could be done without having an `std` feature, but it turns out that `bytes` has a bunch of special additional integration with `std::io` stuff, e.g. `std::io::Reader` and `std::io::IoSlice`. To make the library work as `no_std` we add an `std` feature which is on by default. When it is off, we compile as `no_std` and make parts of the API that require `std::io` conditional on the `std` feature.
@cbeck88 I made some changes to your PR. |
okay, but to my understanding there is no separate target to "link against". the symbols exported in std are simply re-exports of what is in that said, i'm happy with these changes if they make this PR acceptable to reviewers also apparently there's more going on here than I realize per @tarcieri playground example so maybe this has some subtle effect |
Hm maybe you're right, I usually do not link to alloc because it is a bit unclear about it's status when building in std enviornment |
per @tarcieri request
@tarcieri @DoumanAsh let me know if you guys see anything else to improve here, i don't have plans to make further changes at this time |
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.
Looks great now!
@tarcieri i realized that my idea about moving b6cb346#diff-c423debf79de47ec6dd3f075e0fc9cdcR901
because now I've been trying to brainstorm a simple solution to this, because I don't want to sabotage long-term goals of core-only heapless version of this crate. I think another approach might be, instead of
or some such, because then This has the downside, though, that you don't get the benefit described in commit message:
A different direction is, maybe Then,
and this is what bytes exports as But in heapless context you wouldn't try to have IDK I don't have personal need of heapless bytes in my own projects right now but I'm interested to keep brainstorming, and interested what others think: @DoumanAsh ^ My feeling is that
If I were ever to write a serialization lib in rust I would probably try to use |
@cbeck88 right now the main solution we have for this particular problem in an embedded context is to used fixed-sized fields for everything in the protos, so e.g. |
@cbeck88 I'm not sure in particular what will be benefit of moving traits themself, but this particular memthod could be made through trait extension rather than moved. i.e. |
@DoumanAsh it's only to try to separate out the ideas and reduce the need for features to configure the package it might be simpler to just say, there will be an if we think that sounds better, it might be a good idea to add an |
@cbeck88 I believe right now it should be quite simple to make the crate no_std compatible. I don't really see much harm in splitting but I'm myself unsure of whether we should fragment such small crate, I'd suggest to hop onto tokio gitter and discuss it |
I was really hopeful initially that this could be done without
having an
std
feature, but it turns out thatbytes
has a bunchof special additional integration with
std::io
stuff, e.g.std::io::Reader
andstd::io::IoSlice
.To make the library work as
no_std
we add anstd
feature whichis on by default. When it is off, we compile as
no_std
and makeparts of the API that require
std::io
conditional on thestd
feature.