Replaced mysterous bool's in opcodes with u8 #147
Conversation
I suppose, looking at the code, a bool is always represented as u8, which is what we want anyway, but since these values are not bools but placeholder integers that must currently always be 0, having them be bools is kind of strange. Replaced the reading/writing of them with VarUint7, which is maybe almost as invalid, but works. Resolves issue paritytech#145.
|
Can we have read of exactly u8? |
|
There's none built in but it would be easy to add. |
|
Although I'm happy to land this changes, it would be cool to do it with exactly |
|
Sure, no problem. |
|
Done. |
|
Oh, sorry, I meant to read not |
|
Ah, I see, this is only the name issue! |
src/elements/primitives.rs
Outdated
| /// 8-bit unsigned integer, NOT encoded in LEB128; | ||
| /// it's just a single byte. | ||
| #[derive(Debug, Copy, Clone)] | ||
| pub struct VarUint8(u8); |
There was a problem hiding this comment.
Could you please rename it to Uint8 to be more aligned with existing Uint32/Uint64?
src/elements/ops.rs
Outdated
| @@ -756,10 +756,10 @@ impl Serialize for Opcode { | |||
| VarUint32::from(offset).serialize(writer)?; | |||
| }), | |||
| CurrentMemory(flag) => op!(writer, 0x3f, { | |||
| VarUint1::from(flag).serialize(writer)?; | |||
| VarUint7::from(flag).serialize(writer)?; | |||
There was a problem hiding this comment.
I guess we want to use Uint8 here (and below and above)
…length Also fixed a couple other small parts I missed.
|
Derp, thank you. I guess I'm a little slow this afternoon. Let me know if there's anything else I missed! |
|
Nice! Thanks for your pull request! |
I suppose, looking at the code, a bool is always represented as u8, which is what we want anyway, but since these values are not bools but placeholder integers that must currently always be 0, having them be bools is kind of strange.
Replaced the reading/writing of them with VarUint7, which is maybe almost as invalid, but works.
Resolves #145.
Sorry I keep PR'ing API-breaking changes! 😅