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

[James Henderson via Phabricator via llvm-commits]

jhenderson added reviewers: dblaikie, jhenderson.
jhenderson 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:
----------------



================
Comment at: llvm/lib/Demangle/DLangDemangle.cpp:16
+//
+// Ported from libiberty library.
+//
----------------



================
Comment at: llvm/lib/Demangle/DLangDemangle.cpp:26
+using namespace llvm;
+using llvm::itanium_demangle::OutputString;
+
----------------
That using namespace shows that `OutputString` needs moving out of the `itanium_demangle` namespace, since D isn't Itanium!


================
Comment at: llvm/lib/Demangle/DLangDemangle.cpp:30
+  OutputString Decl;
+  char *Demangled = nullptr;
+
----------------
Move this down to close to its first usage.


================
Comment at: llvm/lib/Demangle/DLangDemangle.cpp:32
+
+  if (MangledName == nullptr || *MangledName == '\0')
+    return nullptr;
----------------
If I follow it correctly, the second half of this if is unnecessary, as the `strncmp` will fail instead.


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


================
Comment at: llvm/lib/Demangle/DLangDemangle.cpp:40-42
+  if (strcmp(MangledName, "_Dmain") == 0) {
+    Decl << "D main";
+  }
----------------
LLVM doesn't use braces on single-line ifs.


================
Comment at: llvm/lib/Demangle/DLangDemangle.cpp:44
+
+  if (Decl.getCurrentPosition() > 0) {
+    Decl << '\0';
----------------
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).


================
Comment at: llvm/lib/Demangle/Demangle.cpp:24
 
+static bool isDLangEncoding(const std::string &MangledName) {
+  return MangledName.size() >= 2 && MangledName[0] == '_' &&
----------------
I'm wondering if we could reuse this function at the start of `dlangDemangle` instead of the second if.


================
Comment at: llvm/unittests/Demangle/DLangDemangleTest.cpp:9
+//
+// Testing input ported from libiberty library.
+//
----------------
This comment is probably going to end up out of place as we go forward, as I expect we'll find other test cases that need checking that aren't part fo the libiberty library.


================
Comment at: llvm/unittests/Demangle/DLangDemangleTest.cpp:20-22
+class DLangDemangleTestFixture
+    : public testing::TestWithParam<std::pair<const char *, const char *>> {
+protected:
----------------
I'd probably just ditch the protected, and make this a struct.


================
Comment at: llvm/unittests/Demangle/DLangDemangleTest.cpp:25-27
+  virtual void SetUp() { Demangled = llvm::dlangDemangle(GetParam().first); }
+
+  virtual void TearDown() { std::free(Demangled); }
----------------



================
Comment at: llvm/unittests/Demangle/DLangDemangleTest.cpp:30
+
+TEST_P(DLangDemangleTestFixture, DLangDemangleTest) {
+  EXPECT_STREQ(Demangled, GetParam().second);
----------------
We need test cases for `nullptr` as the input string, and probably something that fails the strncmp, e.g. `_` and `_Z` or similar.


================
Comment at: llvm/unittests/Demangle/DemangleTest.cpp:25
   EXPECT_EQ(demangle("_RNvC3foo3bar"), "foo::bar");
+  EXPECT_EQ(demangle("_Dmain"), "D main");
   EXPECT_EQ(demangle("__RNvC3foo3bar"), "foo::bar");
----------------
Probably move this above or below the two Rust mangling cases.


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

https://reviews.llvm.org/D111414