[PATCH] D111414: [Demangle] Add minimal support for D programming language

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: llvm/unittests/Demangle/DLangDemangleTest.cpp:19
+
+TEST(DLangDemangle, Success) {
+  struct ExpectedVal {
----------------
dblaikie wrote:
> ljmf00 wrote:
> > jhenderson wrote:
> > > dblaikie wrote:
> > > > ljmf00 wrote:
> > > > > dblaikie wrote:
> > > > > > I think most of the mangling testing is done via llvm-cxxfilt - see llvm/test/Demangle/rust.test - so probably best this testing be done that way too.
> > > > > I have a point on this that would like to take into consideration. Since these are unitary tests, I guess this has a better fit here rather than on the cxxfilt integration tests, even though majorly all the demangling tests are written there. I don't know if there is any rationale to test the behaviour on the integrations tests, but since these can all be isolated and therefore tested in a more pure way I think we should move them here, and only test if the path to this demangler is correct on cxxfilt.
> > > > Generally LLVM has small enough tools/binaries that can test functionality in sufficient isolation that most testing is done via these tools (general data structures like those in llvm/ADT are tested with unit tests, for instance - and a few other places where API usage is broad/varied and testing quirks (like different kinds of error handling/corner cases/etc) is valuable).
> > > > 
> > > > In some ways this can be simpler to work with (eg: nothing needs to be recompiled when the test is modified, extracting a command line to run under a debugger is clear) & partly it's what LLVM developers/development are used to.
> > > In my personal opinion gtest testing makes more sense for testing demangling than lit tests. If nothing else, it's probably significantly more efficient, especially on Windows, where the process launching overhead of spawning llvm-cxxfilt and FileCheck is going to be far greater than the time spent running the actual testing itself.
> > If we choose to move to llvm-cxxfilt tests, I will still maintain minimal testing here, mimicking what Rust is currently doing, otherwise I will need to squash https://reviews.llvm.org/D110577 . I agree with @jhenderson for performance reasons although I don't know if it is a point strong enough for changing the testing workflow. Is that spawn process overhead considerable changing regarding the recompilation times? If the case is something like x10 slower per test, I would say that isn't worth changing to cxxfilt test suite.
> Generally this'd only be a single test file with one llvm-cxxfilt invocation and one FileCheck invocation so the overhead shouldn't be too great (yeah, I'd push back against splitting it into a bunch of run lines and separate lit tests).
> 
> Part of my motivation here is consistency, among other reasons, for test discovery - having tests in the same place across the different demanglers, and across LLVM generally, hopefully gives us some chance of being able to maintain tests - add new test cases to the same place, rather than creating another test file because we can't find a good home to add things to.
Yeah, I think we'll want both - at least a small unit test to demonstrate the API usage (as the other demanglers have, I think?) and the llvm-cxxfilt test to demonstrate that's wired up correctly too.

Then the question is where we put all the functional test cases - since we'll end up with both API and command line tests. And I'd prefer those tests live in a similar place/way to the other demanglers (& the way LLVM tends to test more broadly), which I think is in llvm-cxxfilt


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111414