[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:
----------------
jhenderson wrote:
> 
Done


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


================
Comment at: llvm/lib/Demangle/DLangDemangle.cpp:26
+using namespace llvm;
+using llvm::itanium_demangle::OutputString;
+
----------------
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.


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


================
Comment at: llvm/lib/Demangle/DLangDemangle.cpp:32
+
+  if (MangledName == nullptr || *MangledName == '\0')
+    return nullptr;
----------------
jhenderson wrote:
> If I follow it correctly, the second half of this if is unnecessary, as the `strncmp` will fail instead.
True. This can also be joined into the next if. Done


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


================
Comment at: llvm/lib/Demangle/DLangDemangle.cpp:40-42
+  if (strcmp(MangledName, "_Dmain") == 0) {
+    Decl << "D main";
+  }
----------------
jhenderson wrote:
> LLVM doesn't use braces on single-line ifs.
Ok, noted. I will try to change this on the stacked patches.


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


================
Comment at: llvm/lib/Demangle/Demangle.cpp:24
 
+static bool isDLangEncoding(const std::string &MangledName) {
+  return MangledName.size() >= 2 && MangledName[0] == '_' &&
----------------
jhenderson wrote:
> I'm wondering if we could reuse this function at the start of `dlangDemangle` instead of the second if.
This can be done already since dlangDemangle performs that check too, I just followed what the other demanglers were doing. I think if this behaviour is documented I don't see the problem, although I also believe that the compiler optimized is smart enough to optimize this pre check.


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

https://reviews.llvm.org/D111414