[PATCH] D112450: support xcoff for llvm-nm

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/tools/llvm-nm/XCOFF/basic.test:1
+# Test the llvm-nm for XCOFF object files.
+# RUN: llvm-nm  %p/Inputs/test_gcc.o | FileCheck --check-prefix=NM-SYM %s
----------------



================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:144
 
+void reportWarning(Error Err, StringRef Input) {
+  assert(Err);
----------------
# Move this function so that it isn't sandwiched by the `error` functions.
# I think you can just use `WithColor::defaultWarningHandler` to replace most of the logic around `handleAllErrors` below.
# For consistency with the `error` functions in this file, I'd just call this function `warn` rather than `reportWarning`.


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:149-150
+
+  // Flush the standard output to print the warning at a
+  // proper place.
+  outs().flush();
----------------
I think this comment is better (please reflow with clang-format as appropriate).


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:926-927
+  Expected<uint32_t> TypeOrErr = I->getType();
+  if (!TypeOrErr)
+    reportWarning(TypeOrErr.takeError(), Obj.getFileName());
+
----------------
You need to handle the case where `TypeOrErr` is in an error state after this function. Returning `'?'` is probably reasonable.

You need a test case that shows this warning is printed too.


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:929
+
+  uint32_t SymType = TypeOrErr.get();
+
----------------
This is the more common way of getting the value from an `Expected`, at least in places I usually read code. I think it's a little clearer too.


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:938
+  Expected<section_iterator> SymSecOrErr = I->getSection();
+
+  if (!SymSecOrErr)
----------------
Delete this blank line.


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:939-940
+
+  if (!SymSecOrErr)
+    reportWarning(SymSecOrErr.takeError(), Obj.getFileName());
+
----------------
As above - you need to return `'?'` or similar, to prevent trying to dereference an `Expected` in a bad state.


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:1695
         S.Size = ELFSymbolRef(Sym).getSize();
+
       if (PrintAddress && isa<ObjectFile>(Obj)) {
----------------
This change is unrelated - please revert it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112450