[PATCH] D71425: [llvm-cxxfilt] Correctly demangle COFF import thunk

[James Henderson via Phabricator via llvm-commits]

jhenderson accepted this revision.
jhenderson added a comment.
This revision is now accepted and ready to land.

LGTM, with or without the function naming, as you wish, but please make the comment change as requested.



================
Comment at: llvm/test/tools/llvm-cxxfilt/coff-import.test:4
 
-CHECK: import thunk for std::future<void>
+# This should demangle to origin string
+RUN: llvm-cxxfilt -n __imp__foo | FileCheck %s --check-prefix=CHECK-STRING
----------------
steven_wu wrote:
> jhenderson wrote:
> > I'm not sure what this string is trying to say? If demangling fails, then it doesn't demangle, it just prints the original string.
> > 
> > Also minor nit for consistency with tests that use '#' at the start of RUN and CHECK lines: many of the newer tests for the LLVM binutils use '##' for comments to make them stand out, please use that here.
> I don't want to reformat the other test cases now. 
Sorry if I was unclear: I'm not asking you to reformat the other test cases, just the new comment you are adding. In other words, this line only should go from '#' -> '##'


================
Comment at: llvm/test/tools/llvm-cxxfilt/coff-import.test:5
+# This should demangle to origin string
+RUN: llvm-cxxfilt -n __imp__foo | FileCheck %s --check-prefix=CHECK-STRING
+
----------------
steven_wu wrote:
> jhenderson wrote:
> > What is this line trying to check? If it is trying to show that "import thunk for" is not printed, then the check is insufficient - you need to show that the check is for the whole line using --match-full-lines. Did this test fail before your change?
> It was demangled to "import thunk for _foo" but it shouldn't demangle. `--match-full-lines` isn't strictly needed for this but adding that can prevent other kind of breakage. 
Ah, yes, you're right. I think I must have misunderstood something somewhere.


================
Comment at: llvm/tools/llvm-cxxfilt/llvm-cxxfilt.cpp:77
 
-static std::string demangle(llvm::raw_ostream &OS, const std::string &Mangled) {
+static std::string demangleWord(const std::string &Mangled) {
   int Status;
----------------
steven_wu wrote:
> jhenderson wrote:
> > Why did you change this name?
> I remove the OS from the function signature because it is no longer needed. Now it conflicts with the `llvm::demangle` in 
> llvm/Demangle/Demangle.h.
> 
> Alternative is to call `::demangle` on the callside. I am fine with either.
Ah, I didn't realise that there was a name clash. I'm happy with either approach, whichever you prefer.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71425