Read/Write traits with no_std #120
Conversation
| @@ -0,0 +1,34 @@ | |||
| MEMORY | |||
There was a problem hiding this comment.
Is there not an easier way to test embedded stuff than four config files just for a test?
There was a problem hiding this comment.
Yes, like #119 (comment)
however, this way is suggested in the rust embedded book from the rust embedded team, so maybe more common for embedded devs?
There was a problem hiding this comment.
I think .cargo/config could be easy removed in favor of command parameters and .github/workflows/embedded.yml could be mixed with the other github actions. Would that be ok?
There was a problem hiding this comment.
I don't know that we need to squash the workflows files down, but if its possible to remove some of the config files it would be nice, especially given a few of them look like half-comments.
There was a problem hiding this comment.
#119 is updated without comments and fewer config files. Could you please review it there?
There was a problem hiding this comment.
superseded by #122
| @@ -186,7 +186,6 @@ impl ToHex for [u8] { | |||
| } | |||
| } | |||
|
|
|||
| #[cfg(any(test, feature = "std"))] | |||
There was a problem hiding this comment.
Wouldn't it be easier to just have a pub function that converts these objects to hex and then only have the FromHex/etc traits on std-mode, that way its usable either way, but we don't take a new dependency just for hex?
There was a problem hiding this comment.
If you are referring to the new dependency core2, that is for Read/Write traits used in impls.rs
If you are referring to alloc, yes it may be worthy to avoid using alloc just for this traits
There was a problem hiding this comment.
I'm referring to core2 (indeed, alloc isn't really a "new" dependency, as its part of std anyway) - an alternative approach to avoid core2 would be to reimplement the io::Write/Read traits as functions, with versions which expose Write/Read if we're building with std. Ultimately a user not using std isn't going to have Write or Read anyway, they just want a byte buffer (or a string, in the case of hex), which we can just give them, instead of exposing the Write or Read traits (which they're unlikely to use anyway - they don't have std so probably no files, just writing into Vecs).
There was a problem hiding this comment.
#122 is not using core2 and it is simply using input function from the Engine
There was a problem hiding this comment.
@TheBlueMatt I don't think it's realistic to expect people not to use Read or Write, these are universal traits used by all sorts of things, and it looks like core2 is the "standard" way to do this without std. So I think we have to bite the bullet and just use it, even though it bumps our MSRV for nostd and even though it's an extra dep.
We could plausibly get away without it here, or maybe feature-gate it even, but for rust-bitcoin I think we'll need it no matter what, unless we want to either disable all the decoding/encoding stuff (i.e. most of the library) or rewrite it.
There was a problem hiding this comment.
Ah, yeah, I see what you're saying. If some 3rd party writes a fn myfunc<W: Write>(thing: W) and we have elsewhere impl<T: OurCustomNoStdWrite> Write for T { ... } then people can use our types when calling myfunc and not care that we did our own weird thing.
@RCasatta I've changed my mind to agree with Matt now.
There was a problem hiding this comment.
rust-bitcoin would need to replace every std::io::Write with our OurCustomNoStdWrite from bitcoin-hashes, is it correct?
(rust-bech32 doesn't use Write, otherwise, it would be somehow out of context here)
There was a problem hiding this comment.
Yeah, this is correct. Similar to what we did had with Encodable prior to replacing that with io::Write
There was a problem hiding this comment.
Interesting discussion. I didn't see this before opening the following PR in rust-bitcoin with a similar approach:
There was a problem hiding this comment.
(note that I rename it to just Write when I import it into the root - in case you are looking at the rest of the PR)
|
superseded by #122 |
on top of #119 (to be rebased)
AFAIK
"It's not possible to move std::io::{Read,Write} into alloc or core because the API depends on OS specific bits (e.g. last_os_error). An option may be to add a new set of Read / Write traits to core, maybe with associated error types, and then try to bridge these to std::io::{Read,Write} using blanket implementations / super traits, but this needs more research."
So, to include read/write traits the trait core2 has been used
the feature
stdhas been replaced by the inverseno_stdbecause it was needed to include the optional core2 depComputing hash from the embedded example