[PATCH] D72973: [llvm-objdump] Use symbol index+symbol name + storage mapping class as label for -D

[Hubert Tong via Phabricator via llvm-commits]

hubert.reinterpretcast added inline comments.


================
Comment at: llvm/test/tools/llvm-objdump/xcoff-disassemble-all.test:5
+# RUN: llvm-objdump -D --symbol-description %p/Inputs/xcoff-section-headers.o | \
+# RUN:   FileCheck --check-prefixes=COMMON,DESC %s
 
----------------
jasonliu wrote:
> hubert.reinterpretcast wrote:
> > @jasonliu, what is the plan for the merge conflict between this and D75131? In particular, would the relocation printing be tested with both the plain and the descriptive output?
> I think ultimately we might want to have variations for
> -D
> -D --symbol-description
> -D -r
> -D -r --symbol-description
> 
> But the testing for `-D -r --symbol-description` is a bit tricky, as I think if we enable that, relocation printing's symbol should be the new format too, which is not implemented. So I'm Okay for that to be left out, or we just put it in and change later. 
What is the difference if effort between needing to rebase this patch anyway and adding the descriptive output for the relocation printing within this patch?


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:16
+
+namespace llvm {
+
----------------
I had mentioned this with another file on this patch; it is now documented: https://llvm.org/docs/CodingStandards.html#use-namespace-qualifiers-to-implement-previously-declared-functions.


================
Comment at: llvm/tools/llvm-objdump/llvm-xcoffdump.h:26
+
+bool isLabel(const object::XCOFFObjectFile *Obj, const object::SymbolRef &Sym);
+
----------------
I am having trouble accepting that `llvm-objdump` has any business injecting symbols directly into the `llvm` namespace. I understand that `llvm-objdump.h` does do so. @jhenderson, would you object if we introduced an `llvm::objdump_impl` namespace?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72973