[PATCH] D113106: demangle xcoff label symbol for llvm-nm

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


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



================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:636
+static Optional<std::string> demangleXCOFF(StringRef Name) {
+
+  if (!Name.empty() && Name[0] == '.')
----------------
Nit: delete blank line at start of function.


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:637
+
+  if (!Name.empty() && Name[0] == '.')
+    Name = Name.substr(1);
----------------
Can XCOFF symbols have an empty name? If so, you need a test case for this. If not, you don't need a `Name.empty()` check (but you might want some sort of assertion instead).


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:740
     if (Demangle) {
-      if (Optional<std::string> Opt = demangle(S.Name, MachO))
+      Optional<std::string> (*Fn)(StringRef Name) = demangle;
+      if (Obj.isXCOFF())
----------------
This is okay for now, but I think an `llvm::function_ref` would be better overall, as it allows in the future for other demangle options that take different argument sets. I also, personally, find the `function_ref` signature easier to read than a function pointer.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113106