[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:10
+/// \file
+/// This file defines a demangler for D programming language as specified in the
+/// ABI specification, available at:
----------------
dblaikie wrote:
> ljmf00 wrote:
> > jhenderson wrote:
> > > 
> > Done
> Looks like this got mangled - ended up as "the for D programming language"" rather than "for the D programming language".
> 
> But I'll probably leave the review for @jhenderson to come back around and check their feedback's been addressed.
Oh, thanks for noticing! I amended in the right pos.


================
Comment at: llvm/lib/Demangle/DLangDemangle.cpp:38
+
+  initializeOutputString(nullptr, nullptr, Decl, 1024);
+
----------------
jhenderson wrote:
> ljmf00 wrote:
> > jhenderson wrote:
> > > Just a thought, but would it make more sense to have this return the OutputString rather than modify it in-place? Probably rename it to `createOutputString` in that case (this would be a separate patch of course).
> > I didn't write this API. I can change it on another patch if you wish.
> I guess it'll depend on how easy it is to change the API. If there is good reason for its current design, it's fine to remain as-is. Either way, not a blocker for this patch.
The current design relies on the common C error handling way. I think the rationale here is because this function can set multiple variables by reference, and returning a reference could be seen as inconsistent. I think returning the reference would be fine if only one value was set.
I just noticed that I should check if that failed or not. Added that check.


================
Comment at: llvm/lib/Demangle/DLangDemangle.cpp:32
+
+  OutputBuffer Decl;
+  initializeOutputBuffer(nullptr, nullptr, Decl, 1024);
----------------
jhenderson wrote:
> `Decl` is a bit of a weird name to me. I'm assuming it's meant to stand for something like `Declaration`? I feel like `Demangled` or even `DemangledName` (for symmetry with `MangledName`) would be clearer.
I can change to Demangled, I don't really know the full rationale behind it but I believe that is a better name indeed.


================
Comment at: llvm/unittests/Demangle/DLangDemangleTest.cpp:30
+INSTANTIATE_TEST_SUITE_P(DLangDemangleTest, DLangDemangleTestFixture,
+                         testing::Values(std::make_pair("_Dmain", "D main"),
+                                         std::make_pair(nullptr, nullptr),
----------------
jhenderson wrote:
> Just a thought: will brace initializers work here (i.e. `{"_Dmain", "D main"}` instead of `std::make_pair(...)` etc)?
That doesn't work, unfortunately.


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

https://reviews.llvm.org/D111414