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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
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
----------------
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.


================
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
+
----------------
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?


================
Comment at: llvm/test/tools/llvm-cxxfilt/coff-import.test:7-8
+
+RUN: llvm-cxxfilt -n __imp__ZSt6futureIvE __imp__ZSt6futureIvE | \
+RUN:    FileCheck %s --check-prefix=CHECK-SPLIT
 
----------------
This isn't a "split" demangling as you call it. This is two separate strings ("__imp__ZSt6futureIvE" and "__imp__ZSt6futureIvE"), which are individually demangled with no string splitting occurring. Did that case fail before?

I think what you need is `RUN: llvm-cxxfilt -n "__imp__ZSt6futureIvE __imp__ZSt6futureIvE"` so that the two names are treated as a single argument.


================
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;
----------------
Why did you change this name?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71425