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

implemented Debug for Tag to print in hex #2

Merged
merged 2 commits into from Aug 19, 2019
Merged

Conversation

ibaryshnikov
Copy link
Contributor

Motivation: provide useful output for searching tag in dictionary.
Before println!("{:?}", tag) would print Tag(32736, 16), after: Tag(0x7FE0, 0x0010), and now it's easy to find E { tag: Tag(0x7FE0, 0x0010), alias: "PixelData"

@Enet4
Copy link
Owner

Enet4 commented Aug 8, 2019

I wasn't quite expecting to receive PRs at this time, but this one is isolated enough for a quick review.

One can add those formatting flags at the upper level, to obtain a similar outcome:

println!("{:04X?}", tag); // Tag(7FE0, 0010)

What we can't do without a custom Debug implementation is to pretty-print the two components of the tag without pretty-printing the whole struct, so it doesn't add the 0x prefix without it. I also recall that the Display impl already provides a familiar formatting for tags: (GGGG, EEEE).

With this in mind, I'm reluctant to make this change, but would like to hear from you whether the methods above solve your particular concern. The standard dictionary should also be searchable at run-time by tag, so an exact textual match might not be needed.

@ibaryshnikov
Copy link
Contributor Author

I did a lot of debugging recently while opening different files and found this addition quite useful. I don't insist of course, I can still patch it locally.
Usually I just do dbg!(). I like the idea with println!("{:04X?}, tag"), I'll try it and submit a feedback later. At first glance in playground:

#[derive(Debug)]
struct Tag(u16, u16);

#[derive(Debug)]
struct Data {
    tag: Tag,
    value: i32,
}

fn main() {
    let tag = Tag(0xFE00, 0xFFFF);
    println!("{:X?}", tag); // Tag(FE00, FFFF)
    let data = Data { tag, value: 123 }; // I'd like to see this `value` as a decimal
    println!("{:?}", data); // Data { tag: Tag(65024, 65535), value: 123 }
    println!("{:X?}", data); // Data { tag: Tag(FE00, FFFF), value: 7B }
    dbg!(&data);
    // [src/main.rs:24] &data = Data {
    // tag: Tag(
    //     65024,
    //     65535,
    // ),
    // value: 123,
}

Copy link
Owner

@Enet4 Enet4 left a comment

Choose a reason for hiding this comment

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

After some thinking and discussion with other Rust developers, I am willing to make an exception and accept the proposed change at this stage, considering that hexadecimal printing of tag components is indeed useful and more reasonable for this particular data type.

Please see the minor tweak suggested below before merging.

core/src/header.rs Outdated Show resolved Hide resolved
Co-Authored-By: Eduardo Pinho <enet4mikeenet@gmail.com>
@ibaryshnikov
Copy link
Contributor Author

I'm happy to hear that! And your suggestion looks clearer

Copy link
Owner

@Enet4 Enet4 left a comment

Choose a reason for hiding this comment

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

Looks good now, thanks!

@Enet4 Enet4 merged commit d6ad08c into Enet4:master Aug 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants