[PATCH] D72973: [llvm-objdump] Use symbol index+symbol name + storage mapping class as label for -D
Digger via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Apr 2 07:01:21 PDT 2020
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
More information about the llvm-commits
mailing list