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

[Digger via Phabricator via llvm-commits]

DiggerLin added inline comments.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:798
+  if (Ret)
+        assert(getNumberOfAuxEntries() >=1 &&
+           "No CSECT Auxiliary Entry is found.");
----------------
hubert.reinterpretcast wrote:
> Sorry for the churn. Seeing it in this context makes me realize that this probably can't be an `assert` in the first place unless if know that the checking was already done. We will need a function without the `assert` to query whether or not we should expect to find a CSECT Auxiliary Entry (`hasCsectAuxExt` is not named well for that purpose). When we request a CSECT Auxiliary Entry, we can assert that we had a reason to expect that there is one and we can also pass an error back to the caller if there isn't one. Whether or not we can propagate said error without too much difficulty will depend on where the caller is.
I think assert(getNumberOfAuxEntries() >=1 is OK ,here , because the function is hasCsectAuxEnt() , The function names means it should at least has one Csect Aux entry. 

we can return Expected<bool> for the function. 

I am prefer to change it in NFC patch.  because there are some place using the function too.(it is not in the scope the patch) . and the return type of function getXCOFFCsectAuxEnt32  maybe need to change to expect<const XCOFFCsectAuxEnt32 *>

Maybe we also need a new function in the NFC patch

bool XCOFFSymbolRef::withCsectAuxEnt() const {
  XCOFF::StorageClass SC = getStorageClass();
  return (SC == XCOFF::C_EXT || SC == XCOFF::C_WEAKEXT ||
          SC == XCOFF::C_HIDEXT);
} 


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:1
 //===-- XCOFFDump.cpp - XCOFF-specific dumper -------------------*- C++ -*-===//
 //
----------------
jhenderson wrote:
> No need for the "C++" part of this line. That's only to help editors know that a header file is in C++ and to enable appropriate syntax highlighting. See https://llvm.org/docs/CodingStandards.html#file-headers.
thanks


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72973