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

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

rsmith marked 3 inline comments as done.
rsmith added inline comments.


================
Comment at: include/llvm/Demangle/ItaniumDemangle.h:144
+  // would construct an equivalent node.
+  //template<typename Fn> void match(Fn F) const;
+
----------------
erik.pilkington wrote:
> Shouldn't this be match(Fn &&)? Otherwise, someone using std::function would get needlessly bad performance by default.
I'm following the convention used by the C++ standard library: function objects are passed by value by default, and users use `std::ref` to have them passed by copy instead. (Pass-by-value is likely better when copying is acceptable and the function object is small or empty.)

That said, I forgot the `std::ref`s in the `dump()` implementation (fixed now). Perhaps that's a good argument to not follow the standard library convention.


================
Comment at: include/llvm/Demangle/ItaniumDemangle.h:1033
 
-  bool hasRHSComponentSlow(OutputStream &S) const override {
-    if (Printing)
----------------
erik.pilkington wrote:
> Why did you delete this?
Oops, regex gone astray.


================
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
----------------
erik.pilkington wrote:
> Do you think it makes more sense from a library perspective to move the special-case rule to the implementation of the canonicalizer?
Here are four example use cases for the visit/match functionality:

1) AST dumping. As this patch shows, `ForwardTemplateReference` is a special case. (We want to visit the resolved node where possible, not the template parameter index, and need to deal with infinite recursion.)
2) AST pretty-printing. (This is really the same as case 1.)
3) Canonicalizing allocation. We don't want to canonicalize `ForwardTemplateReference` based on constructor arguments, because two nodes with the same constructor arguments do not have the same meaning in general.
4) Cloning / tree transformation. We do not want to clone `ForwardTemplateReference` by calling the constructor with the same constructor arguments, because that would create an unresolved node.

Every reasonable visitor I've considered wants to special-case `ForwardTemplateReference`. It is fundamentally different from the other nodes, because it is not immutable after construction, so there is no way to produce a set of constructor arguments that produce an identical node.

So I think it's better to force every caller of `match` to think about this case by not providing a default implementation. Maybe adding a deleted version would be better though, to make it easier to see that this is intentional when the client hits the inevitable build break? I'll do that.


================
Comment at: include/llvm/Demangle/ItaniumDemangle.h:1769
 
-class BumpPointerAllocator {
-  struct BlockMeta {
----------------
erik.pilkington wrote:
> I think you forgot to add a file that contained these definitions?
This patch doesn't delete the existing lib/Demangle/ItaniumDemangle.cpp; this is still present there. Phabricator seems to hide its contents by default, though, so maybe that's why you're not seeing it?


================
Comment at: lib/Demangle/ItaniumDemangle.cpp:240-241
+
+namespace {
 class BumpPointerAllocator {
   struct BlockMeta {
----------------
(The `BumpPointerAllocator` implementation is over here. A diff algorithm somewhere seems to have got confused, though; this code is unchanged from the prior version in this file.)


Repository:
  rL LLVM

https://reviews.llvm.org/D50930