[PATCH] D99981: [demangler] Support the new Rust mangling scheme (v0)

Thu Apr 22 01:14:12 PDT 2021

jhenderson added inline comments.

================
Comment at: llvm/test/tools/llvm-cxxfilt/rust.test:1
+RUN: llvm-cxxfilt -n  < %s | FileCheck --match-full-lines %s
+
----------------
dblaikie wrote:
> jhenderson wrote:
> > llvm-cxxfilt tool tests are not the place to test the full library functionality.  - that should be tested in the unittests. This test should contain just enough to cover the behaviour implemented in llvm-cxxfilt itself.
> I'd disagree here - generally if there's a fairly simple/thin wrapper around a library, we do test it via lit/command line tests rather than unit tests. And that seems to be how llvm-cxxfilt/Demangle is tested currently (most cases seem to be tested in llvm/test/tools/llvm-cxxfilt, some in llvm/test/Demangle (using llvm-cxxfilt), but not much is tested in llvm/unittests/Demangle.
Not sure I agree with that characterisation. With the exception of the abitag.test test in tools/llvm-cxxfilt, the tests are all primarily testing llvm-cxxfilt level functionality (indirectly some exercise parts of the demangling code too, but I think that's not their purpose).

I don't mind if it makes sense to test things at a lit level, but they should at least be in llvm/test/Demangle. That being said, a unit test invocation for this should simpler than writing a lit test. See for example `DemangleTest.cpp`, where each case is a single line - a lit test case would require at least two process executions (llvm-cxxfilt and FileCheck), and a FileCheck CHECK pattern, so will inevitably longer. Anecdotally, I also feel like it would be easier to make mistakes in the lit test, although that could be resolved relatively easily by using the right FileCheck flags.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99981