Embed minimal required no_std_io2 traits#411
Conversation
hanabi1224
force-pushed
the
embed-nostd-io
branch
from
7997b91 to
d2b2fc8
Compare
|
As it's not as urgent as getting a quick fix out, other things came in between. I'll review it next week. |
As the no-std `Error` is a trimmed down version specifically for this library, it can be simplified. Also add some doc comments back in.
vmx
left a comment
vmx
left a comment
There was a problem hiding this comment.
I've pushed a new commit, as it was easier to do it that way than leaving a comment. A review from you on my changes would be great.
Though I've one comment left, where I don't understand why you made it std only.
| ($name:ident) => { | ||
| impl<const S: usize> no_std_io2::io::Write for $name<S> { | ||
| fn write(&mut self, buf: &[u8]) -> no_std_io2::io::Result<usize> { | ||
| #[cfg(feature = "std")] |
There was a problem hiding this comment.
Wouldn't it make sense to just add something like this at the top:
#[cfg(feature = "std")]
use std::io;
#[cfg(not(feature = "std"))]
use crate::no_std_io as io;There was a problem hiding this comment.
Fixed.
(Did not implement the custom Write trait to not allow downstream crates to depend on it. But that might not be an issue.)
(Top-level use may not work when the macro is used in another file, using the full trait name instead)
There was a problem hiding this comment.
Did not implement the custom
Writetrait to not allow downstream crates to depend on it.
That's actually a good point. My thought was "just keep the current behaviour" without questioning it. I don't have much experience with no-std environments. I guess you would probably implement your own Write-like trait that fits your purpose. You wouldn't want to take a random, stripped down implementation from multihash.
@hanabi1224 it sounds like you've more experience with no-std. So sorry for the back and forth, but I think it makes sense to implement Write only for std. If if it turns out to be a problem for downstream users, they'll tell us.
hanabi1224
force-pushed
the
embed-nostd-io
branch
from
056c2a9 to
f9ef162
Compare
vmx
left a comment
vmx
left a comment
There was a problem hiding this comment.
Thanks for the PR and so quick responses!
2 participants
This PR embedded minimal
no_std_io2traits that are required bymultihash. It should require a minor version bump