[PATCH] D50930: Move Itanium demangler implementation into a header file and add visitation support.

[Richard Smith - zygoloid via Phabricator via llvm-commits]

rsmith added inline comments.


================
Comment at: include/llvm/Demangle/ItaniumDemangle.h:29
 
-namespace {
+#define FOR_EACH_NODE_KIND(X) \
+    X(NodeArrayNode) \
----------------
dlj wrote:
> Could you document the ordering semantics here?
> 
> I *think* it's most-to-least derived, but if I'm wrong... well, there you go. :-)
There's no ordering constraints. As a matter of cleanliness, keeping these in the same order as the class definitions might be nice, but nothing relies on that. After Erik's request to remove the `Expr` class, there isn't any interesting hierarchy here either -- these classes all derived directly from `Node`.


================
Comment at: include/llvm/Demangle/ItaniumDemangle.h:1121
 
 struct ForwardTemplateReference : Node {
   size_t Index;
----------------
dlj wrote:
> Could you add an example of where this type appears?
Sure, why not. (I'll do this in a separate commit.)


================
Comment at: include/llvm/Demangle/ItaniumDemangle.h:1262
     case SpecialSubKind::allocator:
       S += "std::basic_string<char, std::char_traits<char>, "
            "std::allocator<char> >";
----------------
dlj wrote:
> Isn't this just std::allocator? (Assuming it's for `Sa`)
Yes, that sure looks like a (pre-existing) bug! As it happens, this is unreachable, since we don't ever form an `ExpandedSpecialSubstitution` for `SpecialSubKind::allocator`. (The `SpecialSubKind::string` case is also wrong and also unreachable.)

I'll fix this in a separate commit.


Repository:
  rL LLVM

https://reviews.llvm.org/D50930