log: Update precision logic #252
Conversation
sdk/log/crate/src/lib.rs
Outdated
| l1.append_with_args(2u8, &[Argument::Precision(u8::MAX)]); | ||
| assert_eq!(&*l1, "0.02".as_bytes()); |
There was a problem hiding this comment.
This behavior is a bit surprising, but I'm having a hard time coming up with a better option.
Maybe we prepend some other character to indicate that it's been truncated at the front? So if you set u8::MAX precision, you get "*002" or "?002" or "^002".
Prepending "..." seems excessive, since it requires allocating two extra bytes that will almost never be used.
As a side note, is there a reason to not support a precision of MAX_DIGITS and return ".002"?
There was a problem hiding this comment.
This behavior is a bit surprising, but I'm having a hard time coming up with a better option.
Maybe we prepend some other character to indicate that it's been truncated at the front? So if you set u8::MAX precision, you get "*002" or "?002" or "^002".
Same here. I like your idea of using a character. We already use @ when the string is truncated at the end. Perhaps ^ is a good choice, since the other kind of imply either any or missing character.
Prepending "..." seems excessive, since it requires allocating two extra bytes that will almost never be used.
As a side note, is there a reason to not support a precision of
MAX_DIGITSand return ".002"?
No reason, we can do that.
There was a problem hiding this comment.
oh right, I forgot about @, we can just use that if you want!
There was a problem hiding this comment.
Refactored the logic, so the precision is not restricted to the size of the type. That allows logging an u8 value with precision 9 for example. This is probably a more predictable behaviour than "truncating" the precision.
The output will still be truncated based on the size of the buffer – e.g.:
let mut logger = Logger::<4>::default();
append_with_args(u8::MAX, &[Argument::Precision(9)]);will result in"0.0@" .
febo
force-pushed
the
febo/precision-cap
branch
from
345576b to
7365278
Compare
joncinque
left a comment
joncinque
left a comment
There was a problem hiding this comment.
Looks great! Only nits from my side.
sdk/log/crate/src/logger.rs
Outdated
| /// The precision cannot be equal to or greater than the maximum number | ||
| /// of digits for the type. If it is, it will be set to the maximum |
There was a problem hiding this comment.
nit: up to you if you want to specify, but it can be set to something greater, but the output will get truncated, right?
There was a problem hiding this comment.
Good catch! I removed the comment since it does not apply anymore. The output will be truncated according to the size of the buffer, so it is independent of the precision.
Problem
When a precision value larger than the maximum number of digits of a type is used, an overflow occurs and leads to out-of-bounds writes. There is also an issue with the truncate limit equal to
0when logging string values.Solution
This PR updates the logic so the precision value is not bound to the size of the type. This allows logging an
u8value with a precision greater than the maximum number of digits that anu8can have. The output still follows the same truncation rules, so it is limited to the size of the logging buffer.It also fixes the truncate limit issue when logging strings by checking that the limit is greater than
0.