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

[Hubert Tong via Phabricator via llvm-commits]

hubert.reinterpretcast added inline comments.


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:27
+  if (SymRef.hasCsectAuxEnt()) {
+    assert(SymRef.getNumberOfAuxEntries() &&
+           "No CSECT  Auxiliary Entry is found.");
----------------
The assertion makes sense. Would it make more sense as part of `hasCsectAuxEnt`? That is, if `hasCsectAuxEnt` were to return true, then it should assert that there is at least one auxiliary entry (of which we would assume that the last is the csect auxiliary entry).

Also, minor nit: Use `>= 1` if that is actually what is being tested for.


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:28
+    assert(SymRef.getNumberOfAuxEntries() &&
+           "No CSECT  Auxiliary Entry is found.");
+    const XCOFFCsectAuxEnt32 *AuxEntPtr = SymRef.getXCOFFCsectAuxEnt32();
----------------
Please use one space only between words.


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:30
+    const XCOFFCsectAuxEnt32 *AuxEntPtr = SymRef.getXCOFFCsectAuxEnt32();
+    return Optional<XCOFF::StorageMappingClass>(AuxEntPtr->StorageMappingClass);
+  }
----------------
Can we avoid the cast entirely?


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:42
+  if (SymRef.hasCsectAuxEnt()) {
+    assert(SymRef.getNumberOfAuxEntries() &&
+           "No CSECT Auxiliary Entry is found.");
----------------
Seeing a second instance of the assert makes me want it moved even more.


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:44
+           "No CSECT Auxiliary Entry is found.");
+    const XCOFFCsectAuxEnt32 *AuxEntPtr = SymRef.getXCOFFCsectAuxEnt32();
+    return AuxEntPtr->isLabel();
----------------
Please don't create variables for expressions where expanding the expression at the single point of use reads just fine. Note that this probably applies to the other `AuxEntPtr` too if the cast could be removed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72973