Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 39 additions & 1 deletion sdk/log/crate/src/lib.rs
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great tests!

Original file line number Diff line number Diff line change
Expand Up @@ -199,9 +199,47 @@ mod tests {

logger.clear();

// This should have no effect.
// This should have no effect since it is a string.
logger.append_with_args("0123456789", &[Argument::Precision(2)]);
assert!(&*logger == "0123456789".as_bytes());

// Setting a precision equal to or greater than the maximum digits should have
// no effect. The value used will be capped at the maximum digits - 1.
let mut l1 = Logger::<50>::default();
let mut l2 = Logger::<50>::default();

l1.append_with_args(2u8, &[Argument::Precision(u8::MAX)]);
assert_eq!(&*l1, "0.02".as_bytes());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_DIGITS and return ".002"?

No reason, we can do that.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh right, I forgot about @, we can just use that if you want!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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@" .

// same as 2 precision
l2.append_with_args(2u8, &[Argument::Precision(2)]);
assert_eq!(&*l1, &*l2);

l1.clear();
l2.clear();

l1.append_with_args(2u16, &[Argument::Precision(u8::MAX)]);
assert_eq!(&*l1, "0.0002".as_bytes());
// same as 4 precision
l2.append_with_args(2u16, &[Argument::Precision(4)]);
assert_eq!(&*l1, &*l2);

l1.clear();
l2.clear();

l1.append_with_args(2u32, &[Argument::Precision(u8::MAX)]);
assert_eq!(&*l1, "0.000000002".as_bytes());
// same as 9 precision
l2.append_with_args(2u32, &[Argument::Precision(9)]);
assert_eq!(&*l1, &*l2);

l1.clear();
l2.clear();

l1.append_with_args(2u64, &[Argument::Precision(u8::MAX)]);
assert_eq!(&*l1, "0.0000000000000000002".as_bytes());
// same as 19 precision
l2.append_with_args(2u64, &[Argument::Precision(19)]);
assert_eq!(&*l1, &*l2);
}

#[test]
Expand Down
12 changes: 11 additions & 1 deletion sdk/log/crate/src/logger.rs
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,10 @@ pub enum Argument {
/// Number of decimal places to display for numbers.
///
/// This is only applicable for numeric types.
///
/// 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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: up to you if you want to specify, but it can be set to something greater, but the output will get truncated, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

/// number of digits minus one.
Precision(u8),

/// Truncate the output at the end when the specified maximum number of characters
Expand Down Expand Up @@ -262,7 +266,13 @@ macro_rules! impl_log_for_unsigned_integer {
.iter()
.find(|arg| matches!(arg, Argument::Precision(_)))
{
*p as usize
// Precision cannot be equal to or greater than the maximum number
// of digits for the type..
if (*p as usize) >= MAX_DIGITS {
MAX_DIGITS - 1
} else {
*p as usize
}
} else {
0
};
Expand Down