[PATCH] D60134: [llvm-nm]Add support for --no-demangle

[James Henderson via Phabricator via llvm-commits]

jhenderson marked 2 inline comments as done.
jhenderson added a comment.

In D60134#1452786 <https://reviews.llvm.org/D60134#1452786>, @grimar wrote:

> And this patch implements the correct behavior of the normal option the first time it seems.


Thanks. I took the behaviour from --no-demangle in llvm-symbolizer, so I can't exactly take all the credit!



================
Comment at: tools/llvm-nm/llvm-nm.cpp:2083
+  // If both --demangle and --no-demangle are specified then pick the last one.
+  if (NoDemangle.getPosition() > Demangle.getPosition())
+    Demangle = !NoDemangle;
----------------
rupprecht wrote:
> Can you add a test case that ensures getPosition() returns the last occurrence?
> e.g. `-C --no-demangle -C` should be demangled because NoDemangle.getPosition() is 1, and Demangle.getPosition() is 2 and not 0 (or whatever indexing it uses)
Sounds good.


================
Comment at: tools/llvm-nm/llvm-nm.cpp:2085
+    Demangle = !NoDemangle;
+
   // llvm-nm only reads binary files.
----------------
grimar wrote:
> I am not sure this piece of code should be right here.
> 
> It is placed before the `llvm::InitializeAll*` calls, what is a bit too far from where the other values (like `OutputFormat`)
> are set. What about moving it below? For example before the `llvm::for_each(InputFilenames, dumpSymbolNamesFromFile);`.
> I would expect such options that have `-no-*` flags to be grouped and together.
Thanks. I placed it here, because the equivalent code in llvm-symbolizer is at this location. I can move it though.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D60134