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

[Hubert Tong via Phabricator via llvm-commits]

hubert.reinterpretcast added inline comments.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1128
 
+static void printIndexNameSmcAsLable(SectionSymbolsTy &Symbols,
+                                     std::string SymbolName, unsigned int SI) {
----------------
jasonliu wrote:
> 1. Label mis-spelled in the function name. And we are not necessarily printing the "label", so the name is misleading. I would suggest to name it as printFullyQualifiedSymbolName. Or, printXCOFFSymbolName.
> 2. This function looks very XCOFF centric, do we want to put it in XCOFFDump.cpp?
> 3. We only want one symbol inside of SectionSymbolsTy, so it does not make much sense to pass in a pointer to all of the symbols inside of the section, and a index to just get one symbol. I would try to typedef std::tuple<uint64_t, StringRef, uint8_t, int64_t> somewhere in the header and use that type here. And that should be the only one parameter you need. We don't need to pass in the SymbolName as well.
> 
`printXCOFFDecoratedSymbolDescription` or `printXCOFFSymbolDescription`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72973