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

Merged
merged 1 commit into from
Oct 4, 2024

Conversation

GrigorenkoPV
Copy link
Contributor

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 458e641
🔍 Latest deploy log https://app.netlify.com/sites/aya-rs-docs/deploys/66fd8d93f1e578000853f7fd
😎 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
Contributor 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
Contributor Author

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

@tamird
Copy link
Member

tamird commented Oct 2, 2024

Rebased this with regenerated API. @alessandrod does this require further changes?

@mergify mergify bot added the test A PR that improves test cases or CI label Oct 2, 2024
Copy link
Member

@vadorovsky vadorovsky left a comment

Choose a reason for hiding this comment

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

Looks great now, thanks!

@vadorovsky vadorovsky merged commit 55ed9e0 into aya-rs:main Oct 4, 2024
27 checks passed
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 test A PR that improves test cases or CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants