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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

In D99981#2713815 <https://reviews.llvm.org/D99981#2713815>, @dblaikie wrote:

> Yep, with you there with regard to putting tests in the right directory. (except the last bit, as I'm more "unit tests when lit tests aren't viable (including significant ambiguity - if the functionality is used in a bunch of different places/ways)")

Fair enough.

>> If I understand it correctly, this is partly for legacy reasons - unit tests haven't been around in LLVM for all that long, so people use what they are used to.
>
> It was/is a fairly intentional choice (in part Chris Lattner was not a fan of unit tests in general (one of the funnier examples: https://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20130819/185033.html )) - unit tests for code that has a lot of out-of-tree API usage, or just a lot of varied usage internally (like ADTs, in both counts) is worth unit testing. But something with a fairly specific use case/pretty thin mapping to a command line tool (like optimizations - the opt tool testing of middle end optimizations, symbolizing - llvm-symbolizer's such a basic wrapper around that functionality that we test the interesting parts of the algorithm that way and only use unit tests for the interesting parts of the API usage)
>
> I mean, the gunit code &  has been in-tree since 2008, so it's not exactly new.

Guess I didn't understand correctly! Thanks.

> Actually I'd favor splitting it the other way - adding the llvm-cxxfilt support first (with a no-op or trivial example working) then adding new demangling functionality in incremental patches adding more lit tests to it. It's not uncommon to add features to command line tools (like llvm-symbolizer) partly to exercise an otherwise API-only feature

Fine by me.

@tmiasko, I think the minimal functionality @dblaikie is suggesting would be something like implementing the bare minimal support for demangling something like `_RC3foo` (assuming that's a valid mangling). That allows you to have the llvm-cxxfilt code and a test file in place. Then in multiple subsequent patches, you gradually expand the functionality to cover other manglings.



================
Comment at: llvm/lib/Demangle/RustDemangle.cpp:25-29
+  if (MangledName == nullptr || (Buf != nullptr && N == nullptr)) {
+    if (Status != nullptr)
+      *Status = demangle_invalid_args;
+    return nullptr;
+  }
----------------
FWIW, I don't think there's a way to test this block without unit-testing, but I'm fine if the bulk wants to be in the lit tests. Same goes with a number of the other potential failure paths.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99981