Conversation
d412b70 to
5bb1b58
Compare
japaric
left a comment
There was a problem hiding this comment.
The implementation looks good to me.
The user interface seems reasonable. I like that a newtype is still needed. I personally prefer not to make super easy to plug in Debug / Display types into the logs as they are bandwidth expensive and Format should be preferred.
One thing that I'm not sure about is the adapters taking ownership of the inner field. It seems that in most cases you'll want to pass a reference as Debug/Display only need a reference to the value. I'm thinking of cases where you may write:
let x = NotCopyThing::new();
info!("{}", Debug2Format(x));
let y = x; // error: value moved in previous line
I think being uniform everywhere and always requiring a reference (rather than having the option to use x in some cases and &x in others) would avoid running into these unintentional moves.
Display hints are, perhaps confusingly, ignored.
yeah, that's a bit unfortunate. Perhaps add a big warning to the API docs of the X2Format types? I don't think we can emit a warning or an error at call site (from the log macro)
| args.push(Arg::Char(c)); | ||
| } | ||
| Type::Debug | Type::Display => { | ||
| // UTF-8 stream without a prefix length, terminated with `0xFF`. |
There was a problem hiding this comment.
"isn't a 0xFF delimiter problematic with characters like BOM / U+FEFF?"
"no, because UTF-8 encoding never uses the 0xff value; each byte (after encoding) contains at least one zero bit"
case in point, BOM is encoded as [239, 187, 191]
ea46731 to
8a95876
Compare
|
bors r+ |
|
Build succeeded: |
This removes heapless, the
constsreexport, and the need to manually specify buffer sizes for theX2Formatadapters. Instead of formatting to an on-stack buffer, these adapters now stream the formatted data over the defmt sink and terminate it with a0xffbyte.Display hints are, perhaps confusingly, ignored. We could pass them to
core::fmtif we like, but that's a non-breaking change and can be done later.No user-facing format type (
{=Debug}) is provided, since I felt that that may cause confusion between{},{:?},{=Debug}and{=Display}, which would all mean different things.Closes #292