[PATCH] D111419: [Demangle] Add support for D types back referencing

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: llvm/unittests/Demangle/DLangDemangleTest.cpp:43-45
+      {"_D8demangle3ABCQeQaaaa1ai", nullptr},
+      {"_D8demangle4ABCiQfQh1aQh", "demangle.ABCi.ABCi.ABCi.a"},
+      {"_D8demangle4ABCiQfQh1aQaaa", nullptr}
----------------
Might be worth some comments explaining what each test is intended to cover. Using similar phrasing to the comments in the implementation source would help cross reference (eg: is this last one meant to test the "LastBackref" bailout, or the "parseType" failure case? (or something else)


================
Comment at: llvm/unittests/Demangle/DLangDemangleTest.cpp:44
+      {"_D8demangle3ABCQeQaaaa1ai", nullptr},
+      {"_D8demangle4ABCiQfQh1aQh", "demangle.ABCi.ABCi.ABCi.a"},
+      {"_D8demangle4ABCiQfQh1aQaaa", nullptr}
----------------
/might/ be easier to follow if each case is tested in a separate mangled name? (helps to see which things are varying, which things are the same, so the intention of each test is clearer?)

Hmm, yeah, I'm not sure I follow what this is testing. It's got several backrefs, perhaps where one would suffice? (similarly with the previous ABC.ABC.ABC.a test, for what it's worth - not quite following that either) & they look like they'd all parse through parseQualified... 8demangle, 4ABCi, Qf, Qh, 1a, Qh - looks all like that would be resolved by parseIdentifier (calling the base case of parseLName for the numerically prefixed ones, and the parseSymbolBackref for the Q ones) called in parseQualified, 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111419