[PATCH] D49043: [llvm-objdump] Add -demangle (-C) option

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: test/tools/llvm-objdump/disassemble-demangle.test:3
+# RUN: llvm-objdump -d -C %t | FileCheck --check-prefix=DEMANGLE %s
+# RUN: llvm-objdump -d %t | FileCheck --check-prefix=NO-DEMANGLE %s
+
----------------
Since you've added support for the string parameter for -C, please add tests cases for that style too, e.g. --demangle=none and --demangle=itanium.

Also, please add long-name versions of this switch in the test (i.e. have both -C and --demangle).


================
Comment at: tools/llvm-objdump/llvm-objdump.cpp:1523
+      StringRef SymbolName = std::get<1>(Symbols[si]);
+      if (Demangle.getValue() != "none") {
+        char *DemangledSymbol = nullptr;
----------------
Would it perhaps make sense to reject/warn for styles that don't make sense?


================
Comment at: tools/llvm-objdump/llvm-objdump.cpp:1532
+        else
+          outs() << '\n' << SymbolName << ":\n";
+
----------------
To avoid us duplicating the printing code (which could lead to divergence, if we decide the format needs to be slightly different at some point), how would you feel about putting this into a small lambda, which can then be called with the corresponding name? Something like:

```
auto PrintSymbol = [](StringRef Name){ outs() << '\n' << Name << ":\n"; };
```


Repository:
  rL LLVM

https://reviews.llvm.org/D49043