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

Fri Apr 23 16:22:15 PDT 2021

dblaikie added a comment.

Figured I'd move this out of line (so it isn't narrowed to half the screen width):

> We could go back and forth on this a while, I feel.
>
> delimiters.test may have multiple manglings, but the demangling behaviour isn't what's being tested in that test. The purpose of that test is to ensure that the right non-alphanumeric characters act as a delimiter between two character sequences for llvm-cxxfilt's demangling process (see rG5cd5f8f2563395f8767f94604eb4c4bea8dcbea0 <https://reviews.llvm.org/rG5cd5f8f2563395f8767f94604eb4c4bea8dcbea0> where it was introduced). Note that actually it's the same very simple mangled name that is demangled in every case. This behaviour is entirely within the llvm-cxxfilt tool code, not the library code.

Fair point - sorry I missed that detail.

> There are similar stories for most of the other tests in tools/llvm-cxxfilt too. In general, it looks like there is very little dedicated testing of the Itanium demangler code, which is probably part of the problem here.

Yep

> In other GNU binutils replacements, we try to ensure tests that are testing the underlying library code are in test/<library name> rather than test/tool/<tool name>. It's not perfect, but it has been a goal. The idea is that developers of a library that is used by multliple tools can find the tests for that library without having to go through potentially multiple different test/tool/ directories. It also helps the test directory structure reflect the llvm code structure better. Example: if I made a change to a Support class, I'd usually add my test to test/Support, if unit tests aren't viable.

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)")

>> This is generally true of LLVM testing - if there's a convenient command line wrapper around the functionality, then lit testing tends to be the focus. (see symbolizer, dwarfdump, objdump, etc)
>
> 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.

> It's also because it's often easier to write the lit test than it is to get the setup right for a unit test, which is perfectly reasonable to me. That being said, I'm not opposed to using lit tests instead of unit tests, if the developer prefers (as long as the lit tests are in the right place),

I have slightly stronger feelings that this is a community norm that I think is good to reinforce/encourage as a matter of uniformity across the project.

Fair point about where the tests go - they do get a bit chaotic - llvm-symbolizer/libDebugInfoDWARF symbolizing functionality tests are split pretty well between llvm/test/DebugInfo and llvm/test/tools/llvm-symbolizer. Not the end of the world, but good to try to improve consistency in this regard.

> although it's worth pointing out that if this patch were to be split up, llvm-cxxfilt support would probably belong in a separate patch, and therefore testing would HAVE to be put into the unit tests.

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

> Maybe I'm not reflecting the wider LLVM view?

That's my take on it/why I chimed in.

> Should this be brought up on llvm-dev?

If you like - I think the source tree and kinds of test we mostly write on a regular basis shows the norms here.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99981