Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(aya-log): print &[u8] using full width #1008

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

GrigorenkoPV
Copy link

Otherwise you cannot distinguish &[1u8, 0u8] from &[0x10u8] (they both become 10)

Copy link

netlify bot commented Aug 5, 2024

Deploy Preview for aya-rs-docs ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 851d3ef
🔍 Latest deploy log https://app.netlify.com/sites/aya-rs-docs/deploys/66d79503af03c500087d0f08
😎 Deploy Preview https://deploy-preview-1008--aya-rs-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@mergify mergify bot added the aya-log Relating to aya-log label Aug 5, 2024
match last_hint.map(|DisplayHintWrapper(dh)| dh) {
Some(DisplayHint::LowerHex) => Ok(LowerHexDebugFormatter::format(self)),
Some(DisplayHint::UpperHex) => Ok(UpperHexDebugFormatter::format(self)),
Some(DisplayHint::LowerHex) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like this change would belong to *HexDebugFormatter and not inline here?

Copy link
Member

Choose a reason for hiding this comment

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

I agree. Let's make these changes here:

aya/aya-log/src/lib.rs

Lines 208 to 219 in ab000ad

impl<T> Formatter<&[T]> for LowerHexDebugFormatter
where
T: LowerHex,
{
fn format(v: &[T]) -> String {
let mut s = String::new();
for v in v {
let () = core::fmt::write(&mut s, format_args!("{v:x}")).unwrap();
}
s
}
}

aya/aya-log/src/lib.rs

Lines 232 to 243 in ab000ad

impl<T> Formatter<&[T]> for UpperHexDebugFormatter
where
T: UpperHex,
{
fn format(v: &[T]) -> String {
let mut s = String::new();
for v in v {
let () = core::fmt::write(&mut s, format_args!("{v:X}")).unwrap();
}
s
}
}

Copy link
Author

@GrigorenkoPV GrigorenkoPV Sep 2, 2024

Choose a reason for hiding this comment

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

I don't think Rust has a formatting option of "print as many zeroes as needed", so it's not really possible to do this in generic context. And since specialization like in C++ does not exist in (stable) Rust (yet?), I had to hardcode the width of 2 somewhere else.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure what you mean but UpperHexDebugFormatter (perhaps misnamed) is only used for for impl Format for &[u8] so with your change now it becomes dead code. So we might as well do the change there?

@GrigorenkoPV
Copy link
Author

I've done what #1008 (comment) suggested and added a test as well

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
aya-log Relating to aya-log
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants