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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: tools/llvm-objdump/llvm-objdump.cpp:1523
+      StringRef SymbolName = std::get<1>(Symbols[si]);
+      if (Demangle.getValue() != "none") {
+        char *DemangledSymbol = nullptr;
----------------
paulsemel wrote:
> jhenderson wrote:
> > Would it perhaps make sense to reject/warn for styles that don't make sense?
> Maybe we should just warn, as we still want the disassembling happen right ?
Warning is fine for me.


================
Comment at: tools/llvm-objdump/llvm-objdump.cpp:1239
+      Demangle.getValue() != "itanium")
+    errs() << "Unsupported demangling style.\n";
+
----------------
I think this should be at command-line processing time, not here, since we should warn for bad patterns, even if no disassembling is required.

Additionally, this will warn every time this function is called, which if llvm-objdump were to accept multiple objects or presumably parse objects in an archive, would be bad.

What do other LLVM tools do when they get warnings? I'd sort of expect this to be prefixed with "warning:" or similar. It might be worth adding a helper, alongside error(), for this use case. You should also make sure that the style of message matches that of errors in the tool (i.e. first letter capitalization and trailing full stop - should they be exist or not?).


Repository:
  rL LLVM

https://reviews.llvm.org/D49043