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

[Luís Ferreira via Phabricator via llvm-commits]

ljmf00 added inline comments.


================
Comment at: llvm/lib/Demangle/DLangDemangle.cpp:36
+/// \note Beware these aren't required to be '\0' terminated
+struct OutputString {
+
----------------
dblaikie wrote:
> Any chance of using an existing stream type (like `llvm::itanium_demangle::OutputStream` which `RustDemangle` also uses?)? Otherwise might be worth a bunch of separate testing of this class - or incrementally adding functionality to it that functionality is used in the patch series, otherwise it's hard to tell that everything's tested if it's added in one go here & significant parts are currently unused.
This type differs from a stream because D demangler needs the ability to prepend to the output string due to how D demangling is designed. Because of that, a stream is not a good fit here and plus adding methods like prepend will make it conceptually not a stream at all.

I will incrementally add parts of it when necessary and add tests for it. Although, if you find any other data structure that might be suitable here, please let me know.


================
Comment at: llvm/unittests/Demangle/DLangDemangleTest.cpp:19
+
+TEST(DLangDemangle, Success) {
+  struct ExpectedVal {
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111414