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

[Digger via Phabricator via llvm-commits]

DiggerLin marked 8 inline comments as done.
DiggerLin added inline comments.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:769
 
+// TODO: The function need to return an error if there is no Csect auxiliary
+// entry.
----------------
jhenderson wrote:
> need -> needs (same below)
> 
> Is there any reason you can't do this in a separate and prerequisite commit?
yes, we can do it separate and prerequisite commit. but We prefer to  have a NFC plan for it after this patch . @hubert.reinterpretcast  


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.h:28
+                                    const object::RelocationRef &RelRef,
+                                    llvm::SmallVectorImpl<char> &Result);
+} // namespace objdump
----------------
hubert.reinterpretcast wrote:
> Ditto `SmallVectorImpl`.
for above several comment, I have different option.
1. the XCOFFDump.h not compile independently. it be included in the file  llvm/tools/llvm-objdump/XCOFFDump.cpp  and
 llvm/tools/llvm-objdump/llvm-objdump.cpp

if I do not include these two files 
#include "llvm/MC/MCDisassembler/MCDisassembler.h"
#include "llvm/Object/XCOFFObjectFile.h"
in the llvm/tools/llvm-objdump/XCOFFDump.h
I have to include above two file into 
llvm/tools/llvm-objdump/XCOFFDump.cpp  and
 llvm/tools/llvm-objdump/llvm-objdump.cpp

and from compiler view,when it compile the  llvm/tools/llvm-objdump/XCOFFDump.cpp , If I change as you suggestion.
compiler has to open file llvm/tools/llvm-objdump/XCOFFDump.h (and all it depend header files ) and opened  "llvm/MC/MCDisassembler/MCDisassembler.h"
and #include "llvm/Object/XCOFFObjectFile.h"  later. I do not think it is more efficient. (header file StringRef will opened several times.) 

and there are a several lines of something like 
using object::XCOFFObjectFile;
using object::SymbolRef;
using object::RelocationRef;
........

I think it is not  as simple as I include these two files here.







Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72973