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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/docs/CommandGuide/llvm-objdump.rst:329
+
+  Add symbol description for disassembly out. 
+
----------------
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**."


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


================
Comment at: llvm/lib/BinaryFormat/XCOFF.cpp:42
+
+  // TODO: need to add a test case for "Unknown".
+  return "Unknown";
----------------
Perhaps also "and other SMC cases", unless those can be easily tested now.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.h:11
 
+#include "llvm-xcoffdump.h"
 #include "llvm/DebugInfo/DIContext.h"
----------------
Does this really need to be included in the header? Can it be 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.
----------------
Licence header is incomplete (missing first line).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72973