[PATCH] D58763: [llvm-objdump] Should print unknown d_tag in hex format

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: tools/llvm-objdump/ELFDump.cpp:179
 
     StringRef Str = StringRef(Elf->getDynamicTagAsString(Dyn.d_tag));
 
----------------
Higuoxing wrote:
> grimar wrote:
> > Should getDynamicTagAsString just return `nullptr` on unknown flag maybe?
> > (It seems used only once in LLVM, for the place below, btw)
> > 
> > @jhenderson, what do you think?
> There are several functions that return "unknown". I am not so confident to change those codes in `/lib`. In my opinion, I think those should return `""`.
I'd say neither is the right thing to do. I actually think just printing the hex number is not good enough. I think it should be something like `<unknown:>0x12345678`, and I would highly recommend doing that inside getDynamicTagAsString(). It seems rather odd to be comparing against "unknown". The alternative would be to change the interface to return an Optional<> instead of a string, but the name of the function just makes me think I wouldn't need to do anything with the output at all.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D58763