Extend formatters: implement boolean compression #65
Conversation
Lotterleben
changed the title
Lotterleben
force-pushed
the
extend_formatters_2
branch
from
056021c to
84504e1
Compare
| impl Format for bool { | ||
| fn format(&self, fmt: &mut Formatter) { | ||
| let t = internp!("{:bool}"); | ||
| fmt.write(&[t, *self as u8]); |
There was a problem hiding this comment.
Hmm, wait, does this actually work in the decoder? It seems like it wouldn't
There was a problem hiding this comment.
Looks like this doesn't decode properly:
#[derive(Format)]
struct Flags {
a: bool,
b: bool,
c: bool,
}
binfmt::info!(
"{:bool} {:?}",
true,
Flags {
a: true,
b: false,
c: true
}
);Error: couldn't decode all data (remaining: [a, c, 10, d])
There was a problem hiding this comment.
I'm actually not sure if this is even using the Format impl, but something is going wrong somewhere 😅
There was a problem hiding this comment.
yup.. 😅 looking into how I can best attack this with a debugger now.
There was a problem hiding this comment.
I'll try to help:
Encoded data: [0x0A, 0x0C, 0x10, 0x0D]
- 0x0A - string index of
"{:bool} {:?}" - 0x0C - timestamp (10 µs)
- 0x10 - string index of
Flagsgeneratead format string (should be"Flags {{ a: {:bool}, b: {:bool}, c: {:bool} }}") - 0x0D - 0b1101 - all 4 compressed bools
There was a problem hiding this comment.
The decoder doesn't seem to handle this correctly. When it encounters the {:?} it recursively calls parse_args, but doesn't do any special handling for the bool decoding state. Maybe it just needs to be passed down, but I'm not sure yet.
There was a problem hiding this comment.
for the record/note for future us:
from what I've gathered stepping through the bools_bool_struct() test, empty_bool_indices is overwritten when parse_args()is recursively called to handle the {:?} that catches our struct, which messes up the whole compression scheme etc. The question is whether we want to compress such nested bools into one u8 or if that's opening a whole can of worms. Will be mulling this over now but I don't expect any useful results before tomorrow morning
There was a problem hiding this comment.
next note: if we don't compress bools in structs like we would any other "flat" arrangement of arguments, the spec is going to get pretty tangled: compressed bools may be split (for example in a bool struct{bool bool} bool situation) and you have to keep track of which compression u8s cover which levels. plus, you may lose compression.
plus– to implement this, we have to keep track of state during encoding, which adds expense in terms of computation and code size.
So, I'll see how I can make flat compression work in the decoder :D
Lotterleben
force-pushed
the
extend_formatters_2
branch
from
4ade024 to
45327f9
Compare
firmware/qemu/src/bin/log.rs
Outdated
| @@ -18,6 +18,23 @@ fn main() -> ! { | |||
| binfmt::info!("Hello {1:u16} {0:u8} {:bool}", 42u8, 256u16, false); | |||
| binfmt::info!("🍕 {:[u8]}", [3,14]); | |||
|
|
|||
| #[derive(Format)] | |||
| struct Flags { | |||
| a: bool, | |||
There was a problem hiding this comment.
note to self: debug output; delete
|
|
||
| ``` rust | ||
| binfmt::error!("x: {:bool}, y: {:bool}, z: {:bool}", false, false, true); | ||
| // on the wire: [1, 0b100] | ||
| // on the wire: [1, 0b001] |
There was a problem hiding this comment.
shouldn't the value be 0b100 to match the bzyx in the comment?
There was a problem hiding this comment.
other way round– the xyz comment is outdated. fixing now
| } | ||
|
|
||
| binfmt::error!("x: {:bool}, {:?}", false, Flags { a: true, b: false }); | ||
| // on the wire: [1, 2, 0b010,] |
There was a problem hiding this comment.
👍 to compressing the whole thing in a single byte.
There was a problem hiding this comment.
unfortunately it's not as easy as I thought...
| @@ -66,6 +66,13 @@ impl Format for &[u8] { | |||
| } | |||
| } | |||
|
|
|||
| impl Format for bool { | |||
There was a problem hiding this comment.
The thread below talks about structs that contain bool. This Format impl is going to appear in generic types like Option<bool> formatted with {:?}. I think this implementation as it is is correct for those types.
There was a problem hiding this comment.
yup but the de/encoding is broken for fmt strings that contain bools in- and outside of a struct because the compression may transcend those type barriers, but the handling in the de/encoder does not.
Lotterleben
force-pushed
the
extend_formatters_2
branch
from
d83eee1 to
f97da7f
Compare
|
👍 waiting a bit in case @japaric has code comments |
src/lib.rs
Outdated
| } | ||
|
|
||
| fn flush_and_reset_bools(&mut self) { | ||
| self.write(&[self.bool_flags]); |
There was a problem hiding this comment.
nit: you can use self.u8(self.bool_flags) here
Lotterleben
force-pushed
the
extend_formatters_2
branch
2 times, most recently
from
fb16131 to
3bbf2b1
Compare
- teach encoder how to serialze bools
- change bool order in compression bytes
- change position of compressed bool from first `{:bool}` to last `{:bool}`
- update docs
- add test for known struct format bug
Lotterleben
force-pushed
the
extend_formatters_2
branch
from
3bbf2b1 to
c5e0bc4
Compare
resolves #29 .
happy to hear feedback esp. on the decoder changes, surely there's a more idiomatic way to do this.
as always, will squash before merging :)