[PATCH] D71835: [llvm-readobj] - Remove an excessive helper for printing dynamic tags.

Mon Jan 13 01:57:57 PST 2020

grimar added a comment.

In D71835#1801042 <https://reviews.llvm.org/D71835#1801042>, @jhenderson wrote:

> > I am not sure we want to follow GNU here. Even if we do, it should be separate patch probably. The new output looks better and closer to GNU anyways, and the code is a bit simpler.
>
> FWIW, I'm not sure I see a benefit from diverging from GNU here. The only thing I can think of is the lack of '0x' potentially making the value confusing, but the value is actually listed in the first column anyway, so that point isn't exactly an issue.

See:

GNU prints:

  0x0000000012345678 (<unknown>: 12345678) 0x8765432187654321
  0x000000006abcdef0 (Operating System specific: 6abcdef0) 0x9988776655443322
  0x0000000076543210 (Processor Specific: 76543210) 0x5555666677778888

We print:

  0x0000000012345678 (<unknown:>0x12345678) 0x8765432187654321
  0x000000006abcdef0 (<unknown:>0x6abcdef0) 0x9988776655443322
  0x0000000076543210 (<unknown:>0x76543210) 0x5555666677778888

The benefit I see is that we do not need to care about printing "Operating System specific" or "Processor Specific" text
and doing actions to align columns after that. I.e. code that prints "<unknown:>" is just a bit simpler.
It is not clear how much useful to say that unknown value is "Operating System specific" or "Processor Specific".
(I am OK to copy GNU's behavior here if we decide we want it, though)

(Seems we need to change "<unknown:>" to "<unknown>: ", btw, it looks like a bug)

> The only thing I can think of is the lack of '0x' potentially making the value confusing, but the value is actually listed in the first column anyway, so that point isn't exactly an issue.

I think having "0x" prefix for hex values in the output is always(?) a good thing. I think it is a GNU output bug and we probably should not be bug-compatible with GNU here.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71835/new/

https://reviews.llvm.org/D71835