Provides -no-demangle as alias for -demangle=false. Motivation: https://bugs.llvm.org/show_bug.cgi?id=40075
I had a look at an option to disable demangling, as I worked on PR40054, but didn't come up with a clean way of handling both --demangle and --no-demangle, using the cl interface. There, I decided to abandon it, as demangling will be off by default, so we don't need to explicitly disable it. Here is different, because demangling is on by default (and the --demangle switch needs to exist for GNU compatibility). Using tablegen would make it possible to have both sensibly (see examples in e.g. LLD), but I don't think we should do that at this point.
Usually speaking in command-line tools, if there are both positive and negative versions of the option (e.g. --demangle and --no-demangle), the last one present on the command-line takes precedence, so I'd see if there's an API for that. If not, or it's not easy to use, you could probably just emit an error if both are specified.
|215 ↗||(On Diff #181984)|
Nit: use double dash to match other options in this file.
|66 ↗||(On Diff #181984)|
You don't need the cl::NotHidden here, as it is not a true alias option.
This should probably have an explicit cl::init(false), to "match" the ClDemangle option.
I agree with you, hence why I filed the bug in the first place. The "=false" syntax is documented on the CommandLine library page (see https://llvm.org/docs/CommandLine.html#boolean-arguments), but I don't think it is documented explicit in the llvm-symbolizer page.
|92 ↗||(On Diff #181984)|
As noted by @ruiu, let's not document "demangle=false" explicitly, so remove the second sentence here.
Update tests. Add logic. Honestly, I'm not good at writing llvm-symbolizer test, so I picked test that would fail if --demangle=true to test --demangle, --no-demangle order.
That doesn't mean I want to merge this patch as is. If more specific tests are needed I would be happy to dive into.
|42–46 ↗||(On Diff #182267)|
I think it would make more sense to extend the case below that you have already added --no-demangle to.
There you can test it something like the following (add the "DEMANGLED_FUNCTION_NAME" alongside the "SHORT_FUNCTION_NAME" check pattern):
RUN: llvm-symbolizer --functions=short --demangle --no-demangle < %t.input7 \ RUN: | FileCheck %s --check-prefix=SHORT_FUNCTION_NAME RUN: llvm-symbolizer --functions=short --no-demangle --demangle < %t.input7 \ RUN: | FileCheck %s --check-prefix=DEMANGLED_FUNCTION_NAME DEMANGLED_FUNCTION_NAME: c()
|211–212 ↗||(On Diff #182267)|
Nice! I didn't know about the getPosition function.
|221 ↗||(On Diff #182283)|
Sorry, I realised that I misread the test. The existing --demangle=false tests are testing that the "short" name is NOT matched (that's what the -NOT suffix is for on the line above, so this isn't testing what you think it is testing.
Instead, you'll want some new tests below this and the corresponding checks. The --functions=short means that it is printing the short version of the function name, taken from the debug information, I think. In the new tests you write, you don't want the --functions switch at all, I believe.
Try experimenting with running llvm-symbolizer on %t.input7 and look at the output. You should see a function name c() appear somewhere, if I'm not mistaken. Assuming you do, double-check that --no-demangle turns it into _Z1cv, and then add something like the following tests at the end of the file:
; Check that the last of --demangle and --no-demangle wins. RUN: llvm-symbolizer --demangle < %t.input7 \ RUN: | FileCheck %s --check-prefix=DEMANGLED_FUNCTION_NAME RUN: llvm-symbolizer --no-demangle < %t.input7 \ RUN: | FileCheck %s --check-prefix=MANGLED_FUNCTION_NAME RUN: llvm-symbolizer --demangle --no-demangle < %t.input7 \ RUN: | FileCheck %s --check-prefix=MANGLED_FUNCTION_NAME RUN: llvm-symbolizer --no-demangle --demangle < %t.input7 \ RUN: | FileCheck %s --check-prefix=DEMANGLED_FUNCTION_NAME MANGLED_FUNCTION_NAME: _Z1cv DEMANGLED_FUNCTION_NAME: c()
|209 ↗||(On Diff #182283)|
Did you mean to remove the comment you had here before?
|210 ↗||(On Diff #182497)|
Nit: remove the "as demangle option".
Also, use clang-format to choose when to split the column onto multiple lines, rather than doing it by hand. I think most (maybe all) of this line will fit on the same line as the first line.
|212 ↗||(On Diff #182497)|
What does getPosition do if the option is not on the command-line?
|214–216 ↗||(On Diff #182497)|
Why has this changed?