[PATCH] D109452: implement the --syms and using "symbol index and qualname" for --sym --symbol--description for llvm-objdump for xcoff

[Digger Lin via Phabricator via llvm-commits]

DiggerLin marked 5 inline comments as done.
DiggerLin added a comment.

In D109452#3003268 <https://reviews.llvm.org/D109452#3003268>, @jhenderson wrote:

> This patch seems to have gained a lot of functionality that sounds like it belongs in separate patches? The patch description and summary talk about reporting the symbol index, the qualname, and the symbol section. Other fields should be done in another patch, I'd think. There's no need for a single patch to implement all necessary functionality needed for complete symbol table printing.

yes, this is a big patch.  Since I have implemented all the functionality in one patch, I do not think it is a good ideal to separate the patch to two patches now. I changed the title of the patch and the summary of the patch as your suggestion.  thanks.



================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:230
+    if (!CsectAuxRefOrError)
+      consumeError(CsectAuxRefOrError.takeError());
+    else
----------------
jhenderson wrote:
> `consumeError` is often a code smell. You should either 1) use `cantFail` instead, 2) add a TODO comment saying to report the message up the stack, 3) report the error somehow, or 4) add a comment explaining why the `consumeError` is justified here (e.g. "we don't care about invalid files for this purpose"). I suspect at this time 2) is the appropriate response.
> 
> Same comment applies for the other `consumeError` instances.
thanks


================
Comment at: llvm/test/tools/llvm-objdump/XCOFF/symbol-table.test:1
+; Test for the option --syms for the llvm-objdump for xcoff object file.
+; Test for the option --syms --symbol-description for xcoff object file.
----------------
jhenderson wrote:
> We usually describe what the test //does// (and why where necessary - not applicable here) not what the test //is//: all the files in this directory are for testing llvm-objdump, so the comment can be simplified somewhat.
thanks


================
Comment at: llvm/test/tools/llvm-objdump/XCOFF/symbol-table.test:7
+
+;; Following IR are generated by following source code.
+;; bash> cat test.c
----------------
jhenderson wrote:
> 
thanks


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.h:25
+getXCOFFSymbolContainingSymbolRef(const object::XCOFFObjectFile *Obj,
+                                  const object::SymbolRef &Sym);
+
----------------
jhenderson wrote:
> DiggerLin wrote:
> > jhenderson wrote:
> > > This is a problem throughout this header file (and its cpp) already, so please make a small commit to fix it: `SymbolRef` is designed to be a lightweight struct and passed by value, not by reference (just like `StringRef`).
> > I think we should make a separate patch do deal the problem after the patch upload. 
> I'm happy as long as it gets down.
thanks


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109452