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

[Erik Pilkington via Phabricator via llvm-commits]

erik.pilkington added a comment.

This seems like a good direction to me! +1 for dropping the vptr from Node, I think I'll try to do that once all the dust settles from these changes. I think you forgot to `git add` the new ItaniumDemangle.cpp into the diff, though.



================
Comment at: include/llvm/Demangle/ItaniumDemangle.h:1
-//===------------------------- ItaniumDemangle.cpp ------------------------===//
+//===------------------------- ItaniumDemangle.h --------------------------===//
 //
----------------
Nit: you forgot the "-*- C++ -*-"


================
Comment at: include/llvm/Demangle/ItaniumDemangle.h:144
+  // would construct an equivalent node.
+  //template<typename Fn> void match(Fn F) const;
+
----------------
Shouldn't this be match(Fn &&)? Otherwise, someone using std::function would get needlessly bad performance by default.


================
Comment at: include/llvm/Demangle/ItaniumDemangle.h:1033
 
-  bool hasRHSComponentSlow(OutputStream &S) const override {
-    if (Printing)
----------------
Why did you delete this?


================
Comment at: include/llvm/Demangle/ItaniumDemangle.h:1136
 
-  bool hasRHSComponentSlow(OutputStream &S) const override {
-    if (Printing)
-      return false;
-    SwapAndRestore<bool> SavePrinting(Printing, true);
-    return Ref->hasRHSComponent(S);
-  }
+  // We don't provide a matcher for these, because the value of the node is
+  // not determined by its construction parameters, and it generally needs
----------------
Do you think it makes more sense from a library perspective to move the special-case rule to the implementation of the canonicalizer?


================
Comment at: include/llvm/Demangle/ItaniumDemangle.h:1416-1418
 struct Expr : public Node {
-  Expr(Kind K = KExpr) : Node(K) {}
+  Expr(Kind K) : Node(K) {}
 };
----------------
May as well get rid of this base class now. (If you don't mind!)


================
Comment at: include/llvm/Demangle/ItaniumDemangle.h:1769
 
-class BumpPointerAllocator {
-  struct BlockMeta {
----------------
I think you forgot to add a file that contained these definitions?


Repository:
  rL LLVM

https://reviews.llvm.org/D50930