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

Wed Apr 21 17:43:02 PDT 2021

dblaikie added a comment.

In D99981#2701057 <https://reviews.llvm.org/D99981#2701057>, @jhenderson wrote:

> If you are looking for a full review of this, try adding specific people as reviewers who have worked on the demangling library. It would also make sense to split this change up into lots of small parts. For example, you could implement a small subset of the Rust demangling scheme, in a first patch, and expand on it in a series of subsequent patches. That'll make each patch easier to review. Connecting it up to llvm-cxxfilt could also be a separate patch from implementing the demangling code.

+1 to all that - incremental patches would probably help here

================
Comment at: llvm/test/tools/llvm-cxxfilt/rust.test:1
+RUN: llvm-cxxfilt -n  < %s | FileCheck --match-full-lines %s
+
----------------
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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99981