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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/lib/Demangle/DLangDemangle.cpp:1
+//===--- DLangDemangle.cpp -------------------------------------*- C++ -*-===//
+//
----------------
The "C++" bit is specifically for header files only, as it's to help some editors identify the file type for .h files.


================
Comment at: llvm/lib/Demangle/DLangDemangle.cpp:33
+
+/// A mini string-handling class for output strings generated by D demangler
+///
----------------
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).


================
Comment at: llvm/lib/Demangle/DLangDemangle.cpp:36
+/// \note Beware these aren't required to be '\0' terminated
+struct OutputString {
+
----------------
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.


================
Comment at: llvm/lib/Demangle/DLangDemangle.cpp:197
+
+/* vim: set ts=2 sw=2 expandtab */
----------------
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.


================
Comment at: llvm/unittests/Demangle/DLangDemangleTest.cpp:19
+
+TEST(DLangDemangle, Success) {
+  struct ExpectedVal {
----------------
dblaikie wrote:
> ljmf00 wrote:
> > dblaikie wrote:
> > > I think most of the mangling testing is done via llvm-cxxfilt - see llvm/test/Demangle/rust.test - so probably best this testing be done that way too.
> > I have a point on this that would like to take into consideration. Since these are unitary tests, I guess this has a better fit here rather than on the cxxfilt integration tests, even though majorly all the demangling tests are written there. I don't know if there is any rationale to test the behaviour on the integrations tests, but since these can all be isolated and therefore tested in a more pure way I think we should move them here, and only test if the path to this demangler is correct on cxxfilt.
> Generally LLVM has small enough tools/binaries that can test functionality in sufficient isolation that most testing is done via these tools (general data structures like those in llvm/ADT are tested with unit tests, for instance - and a few other places where API usage is broad/varied and testing quirks (like different kinds of error handling/corner cases/etc) is valuable).
> 
> In some ways this can be simpler to work with (eg: nothing needs to be recompiled when the test is modified, extracting a command line to run under a debugger is clear) & partly it's what LLVM developers/development are used to.
In my personal opinion gtest testing makes more sense for testing demangling than lit tests. If nothing else, it's probably significantly more efficient, especially on Windows, where the process launching overhead of spawning llvm-cxxfilt and FileCheck is going to be far greater than the time spent running the actual testing itself.


================
Comment at: llvm/unittests/Demangle/DLangDemangleTest.cpp:25
+
+  ExpectedVal ExpectedArray[] = {
+      {"_Dmain", "D main"}
----------------
gtest has specific support for parameterised tests (see the `TEST_P` macro), so it probably makes more sense to leverage that rather than hand-roll your own looping logic. This is particularly important, because it impacts how test failures are reported.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111414