Always respect DisplayHint::Ascii for byte slices #321
Conversation
Sh3Rm4n
force-pushed
the
ascii
branch
2 times, most recently
from
a258daf to
d34c8d9
Compare
| _ => None, | ||
| }) | ||
| .collect::<Vec<u8>>(); | ||
| format_bytes(&vals, hint, &mut buf)? |
There was a problem hiding this comment.
When formatting a slice of structs (or anything that isn't a u8) with {=[?]:a}, I'd expect the display hint to be ignored, but the code looks like it'll instead output nothing.
There was a problem hiding this comment.
For example, you might want to format this struct with {=[?]:a} in order to display the name field as an ASCII string:
struct Data<'a> {
name: &'a [u8],
value: bool,
}
|
I've resolved the concerns in #321 (comment), but I'm still making assumptions, where I'm not sure if they hold true for every case:
And I feel, that this code can be written a bit more elegant / readable. But it works for now :) |
There was a problem hiding this comment.
Thanks for the PR. This looks good. I left a suggestion.
Regarding your questions / assumptions
If any element in a slice has the "{=u8}" format, does that mean, that all elements have this format?
Yes. The format string (field) can vary from element to element (of a format slice) but only if we are formatting a slice of enums. In that case, the format string is the variant and not the complete enum (e.g. Foo({:u8}) (variant) vs Foo({:u8})|Bar({:u16}) (enum)). The format string of a single variant cannot be {=u8}, however.
Does "{=u8}" format ensure, that the values are also always in u8 range?
Yes, because the macros do type checking: the value must have u8 type.
Do FormatSlice elements for "{=u8}" always contain only one value, and when is this not true?
Yes.
decoder/src/lib.rs
Outdated
| let vals = elements | ||
| .iter() | ||
| // Assumption: Arguments for u8 has always only 1 element | ||
| .filter_map(|e| match e.args[0] { |
There was a problem hiding this comment.
I would suggest using map instead of filter_map and panicking if arg is not a single Arg::Uxx. Because: if for some reason (i.e. a bug) the program reaches that state then the current logic will silently ignore the errors and print the wrong thing. With a panic the bug will be immediately exposed.
There was a problem hiding this comment.
Converted both assumptions to panics, if they are not true.
This is a naive implementation, which assumes, that the `Uxx` value
is always in `u8` range. It does not consider the given format type `{=$type}`
Also, it assumes that the element args array only contains one element
|
Rebased and squashed. Ready for review :) |
|
bors r+ |
|
Build succeeded: |
This is a naive implementation, which assumes, that the
Uxxvalueis always in
u8range. It does not consider the given format type{=$type}Also, it assumes that the element args array only contains one element
Fixes #318