[PATCH] D120357: [llvm-nm]add helper function to print out the object file name, archive name, architecture name

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Something very strange just happened and half my review comments were lost in my attempt to post. Sorry if anything ends up getting duplicated.



================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:1745
 
-static void dumpSymbolNamesFromObject(SymbolicFile &Obj, bool printName,
-                                      StringRef ArchiveName = {},
-                                      StringRef ArchitectureName = {}) {
-  if (!shouldDump(Obj))
-    return;
-
-  if (ExportSymbols && Obj.isXCOFF()) {
-    XCOFFObjectFile *XCOFFObj = cast<XCOFFObjectFile>(&Obj);
-    getXCOFFExports(XCOFFObj, ArchiveName);
-    return;
-  }
-
+// Returns false if there is error found or true otherwise.
+static bool getSymbolNamesFromObject(SymbolicFile &Obj) {
----------------



================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:1750-1763
     const auto *E = dyn_cast<ELFObjectFileBase>(&Obj);
     if (!E) {
       error("File format has no dynamic symbol table", Obj.getFileName());
-      return;
+      return false;
     }
     Symbols = E->getDynamicSymbolIterators();
 
----------------
Moving the conversation from below: I think you should still use `getDynamicSyms` here and just use another `cast` call to get the ELF object again, as in the inline edit. It's reasonably straightforward, and removes the duplicated error.


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:1774
     if (Nsect == 0)
-      return;
+      return false;
   }
----------------
Are you sure this should be `return false`? (I don't know either way, but we should preserve behaviour).


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:1876
+  if (DynamicSyms)
+    return !cantFail(getDynamicSyms(Obj)).empty();
+  return !Obj.symbols().empty();
----------------
DiggerLin wrote:
> I use cantFail because if there is a error , it will happen at function getSymbolNamesFromObject first.  it will not go to  hasSymbols() which be called in the function dumpSymbolNamesFromObject. using cantFail  let code simple here.
I'm not particularly comfortable with this, because it would be trivially easy for someone in the future to add another `hasSymbols` call site, without having first called `dumpSymbolNamesFromObject`. I think it would be better to return an `Expected<bool>` from this function, and then use `cantFail` at the relevant call site(s), possibly with a comment at those sites explaining why the function cannot fail.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120357