[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:1
+//===--- DLangDemangle.cpp -------------------------------------*- C++ -*-===//
+//
----------------
jhenderson wrote:
> The "C++" bit is specifically for header files only, as it's to help some editors identify the file type for .h files.
Done


================
Comment at: llvm/lib/Demangle/DLangDemangle.cpp:33
+
+/// A mini string-handling class for output strings generated by D demangler
+///
----------------
jhenderson wrote:
> Nit: here and in other comments, we end our comments with a `.`
> 
> Similarly, comments should always start with a capital letter (already done here, but not done further down).
Done


================
Comment at: llvm/lib/Demangle/DLangDemangle.cpp:36
+/// \note Beware these aren't required to be '\0' terminated
+struct OutputString {
+
----------------
dblaikie wrote:
> jhenderson wrote:
> > dblaikie wrote:
> > > ljmf00 wrote:
> > > > 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.
> > > Ah, fair enough. Might want to check the naming conventions (I'd expect "need" to be called "reserve" in C++ API parlance, though perhaps in LLVM's APIs that's called "grow"?)/API design (do the other similar data structures have a "free()" function, or do they rely on the dtor to cleanup?) line up with the existing OutputStream/other data structures in LLVM for consistency.
> > > 
> > > & maybe worth pulling it out into a separate header - given how big this whole file is likely to get?
> > I wonder if we could model this as some kind of `deque<std::string>` - basically buffer things by adding strings to the start or end of the queue (prepend and append), before finalizing it into a single string at a later point. It might help avoid a lot of manual memory management/copying etc.
> Probably would be a `deque<char>`? Though that'd then involve copying the result into the output buffer - so I think the current approach is probably a/the good one, and consistent with the other demanglers that write directly into their output buffer. Just with the added constraint of needing to be able to prepend info.
> 
> (another alternative would be to generalize the existing itanium OutputStream to support prepending too)
I think a deque is not a good fit for this for several reasons. I think you were referring to some sort of `deque<char*>`, otherwise it would be very memory inefficient -- the deque node structs are way heavier than a byte.

Even with a string deque memory not being contiguous across nodes can also hurt performance, when, e.g. calculating the length of a given deque. Another problem with that is the fact that memory allocation is done in chunks and gown twice when needed.

Also, assuming that a string is appended/prepended to the deque on every OutputString::append or OutputString::prepend, allocating small chunks individually is also incredibly slow -- if that is not the case I'm not seeing a way where it is applicable at all.

Talking about the OutputStream, as I mentioned above, prepending is not part of a Stream concept (correct me if I'm wrong), since a stream is designed to only grow at the end. I'm not quite sure if something is relying on specific characteristics of a stream but, e.g. because a stream only grows at the end, adding a prepend would mess up with indices. If that is not a problem, I would consider that option but also renaming it.


================
Comment at: llvm/lib/Demangle/DLangDemangle.cpp:197
+
+/* vim: set ts=2 sw=2 expandtab */
----------------
jhenderson wrote:
> I don't think LLVM includes these sorts of comments (with the exception of the special C++ annotation in the first line of the license header in header files.
Sorry and thanks for noticing, I probably added this because my editor wasn't smart enough while porting


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

https://reviews.llvm.org/D111414