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

[Digger via Phabricator via llvm-commits]

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


================
Comment at: llvm/docs/CommandGuide/llvm-objdump.rst:329
+
+  Add symbol description for disassembly out. 
+
----------------
jhenderson wrote:
> Please be a little more careful when addressing review comments, to avoid repeated churn. You haven't changed to my text as requested:
> 
> "Add symbol description **to** disassembly **output**."
thanks. 


================
Comment at: llvm/include/llvm/MC/MCDisassembler/MCDisassembler.h:38
+  };
+  bool IsXCoff;
 
----------------
jhenderson wrote:
> `IsXCoff` might want to be `IsXCOFF`, since `Coff` is an acronym. Also, it probably wants to be a private member?
OK, thanks


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.h:11
 
+#include "llvm-xcoffdump.h"
 #include "llvm/DebugInfo/DIContext.h"
----------------
jhenderson wrote:
> Does this really need to be included in the header? Can it be in llvm-objdump.cpp only?
yes, it can put in llvm-objdump.cpp only.


================
Comment at: llvm/tools/llvm-objdump/llvm-xcoffdump.h:1
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
----------------
jhenderson wrote:
> Licence header is incomplete (missing first line).
thanks


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72973