Page MenuHomePhabricator

[lib/Support/YAMLTraits] - Don't print leading zeroes when dumping Hex8/Hex16/Hex32 types.
ClosedPublic

Authored by grimar on Fri, Nov 6, 5:15 AM.

Details

Summary

When we produce an YAML output, we also print leading zeroes currently.
An output might look like this:

- Name:    .dynsym
  Type:    SHT_DYNSYM
  Address: 0x0000000000001000
  EntSize: 0x0000000000000018

There are probably no reason to print leading zeroes.
It just makes harder to read values. This patch stops printing them.
The output becomes like:

- Name:    .dynsym
  Type:    SHT_DYNSYM
  Address: 0x1000
  EntSize: 0x18

This affects obj2yaml mostly, but also dsymutil and llvm-xray tools output.

Diff Detail

Event Timeline

grimar created this revision.Fri, Nov 6, 5:15 AM
Herald added a project: Restricted Project. · View Herald Transcript
grimar requested review of this revision.Fri, Nov 6, 5:15 AM

I think it would be a good idea to get llvm-xray and dsymutil developers to give their opinion on this change. From the obj2yaml side of things, it looks fine to me, but the use-cases are different for the other tools.

llvm/lib/Support/YAMLTraits.cpp
1049

I'm assuming these formats are printf format strings, in which case you should be using the PRIX8/PRIX16/PRIX32/PRIX64 macros, I think.

MaskRay added a subscriber: dberris.Fri, Nov 6, 8:17 PM

Good idea to me.

+@dberris for llvm-xray side changes.

grimar updated this revision to Diff 303810.Mon, Nov 9, 3:21 AM
grimar marked an inline comment as done.
  • Rebased, addressed review comment.

I think you uploaded a messed-up diff? There are lldb changes that have nothing to do with this patch.

grimar planned changes to this revision.Mon, Nov 9, 5:37 AM

I think you uploaded a messed-up diff? There are lldb changes that have nothing to do with this patch.

Oh. Thanks!

grimar updated this revision to Diff 303840.Mon, Nov 9, 5:40 AM
  • Uploaded correct diff.
jhenderson accepted this revision.Mon, Nov 9, 5:47 AM

I think it would be a good idea to get llvm-xray and dsymutil developers to give their opinion on this change. From the obj2yaml side of things, it looks fine to me, but the use-cases are different for the other tools.

LGTM, from an obj2yaml side of things, but this point still applies.

This revision is now accepted and ready to land.Mon, Nov 9, 5:47 AM
grimar updated this revision to Diff 305761.Tue, Nov 17, 5:50 AM

Ping.

  • Rebased.
MaskRay accepted this revision.Tue, Nov 17, 10:30 PM

@grimar this one slipped through https://github.com/llvm/llvm-project/commit/8c1e3cbebfe9ff14829a279b9d229d4fc3f190b1

unrelated, but I wonder how your diff https://reviews.llvm.org/D90930?id=303810 ended up without repository assigned? (e.g. builds didn't run)

grimar added a comment.EditedThu, Nov 19, 12:26 AM

Oh, sorry about that. This because its test has "REQUIRES : system-darwin" I think. It just doesn't run on my windows/linux systems.

unrelated, but I wonder how your diff https://reviews.llvm.org/D90930?id=303810 ended up without repository assigned? (e.g. builds didn't run)

When I create a new diff, I fill "Title", "Summary", "Reviewers" and "Subscribers". I never tried to fill "Repository" field by myself. Honestly I've just
realized that this is the reason why Harbormaster doesn't build diffs from all my reviews. I just didn't know how this feature exactly works.
Will try next time :)