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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Just a few nits left from me.

A note on the commit message: we don't usually add `Signed off by` tags to the commit messages in LLVM.



================
Comment at: llvm/lib/Demangle/DLangDemangle.cpp:26
+using namespace llvm;
+using llvm::itanium_demangle::OutputString;
+
----------------
ljmf00 wrote:
> jhenderson wrote:
> > That using namespace shows that `OutputString` needs moving out of the `itanium_demangle` namespace, since D isn't Itanium!
> This is the same on Rust demangler, that is primarily why I didn't write a patch. I can do the renaming on another separate patch.
Sounds reasonable, thanks.


================
Comment at: llvm/lib/Demangle/DLangDemangle.cpp:38
+
+  initializeOutputString(nullptr, nullptr, Decl, 1024);
+
----------------
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.


================
Comment at: llvm/lib/Demangle/DLangDemangle.cpp:44
+
+  if (Decl.getCurrentPosition() > 0) {
+    Decl << '\0';
----------------
ljmf00 wrote:
> jhenderson wrote:
> > Perhaps add a comment or probably put this in a separate function, as it feels like a different level of abstraction to the rest of the code in this fucntion, and I don't really understand why it's necessary (NB: I haven't looked into the OutputString interface, but ideally, I shouldn't have to).
> OutputString/OutputBuffer is not null terminated therefore this is necessary. This is also being done on other demanglers to make the API conformat with null terminated strings. I don't belive such tiny thing need a separate function, but I can generalize this if it is really needed.
The separate function thought was mostly a way of describing what this code is doing, as it's not immediately clear if you aren't familiar with the `OutputBuffer` interface. A simple comment at the start of the if is probably sufficient, if you prefer that though.


================
Comment at: llvm/lib/Demangle/DLangDemangle.cpp:32
+
+  OutputBuffer Decl;
+  initializeOutputBuffer(nullptr, nullptr, Decl, 1024);
----------------
`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.


================
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),
----------------
Just a thought: will brace initializers work here (i.e. `{"_Dmain", "D main"}` instead of `std::make_pair(...)` etc)?


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

https://reviews.llvm.org/D111414