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

[Erik Pilkington via Phabricator via llvm-commits]

erik.pilkington accepted this revision.
erik.pilkington added a comment.

LGTM too, thanks for doing this!



================
Comment at: include/llvm/Demangle/ItaniumDemangle.h:144
+  // would construct an equivalent node.
+  //template<typename Fn> void match(Fn F) const;
+
----------------
rsmith wrote:
> 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.
Oh, okay, I guess it makes more sense to follow the standard conventions here then.


================
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
----------------
rsmith wrote:
> 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.
Okay, I agree then, that's pretty convincing.


================
Comment at: include/llvm/Demangle/ItaniumDemangle.h:1262
     case SpecialSubKind::allocator:
       S += "std::basic_string<char, std::char_traits<char>, "
            "std::allocator<char> >";
----------------
rsmith wrote:
> 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.
Oops! Since this was unreachable, we should just remove these cases from the `switch`.


================
Comment at: include/llvm/Demangle/ItaniumDemangle.h:1769
 
-class BumpPointerAllocator {
-  struct BlockMeta {
----------------
rsmith wrote:
> 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?
Oh, right, my mistake!


================
Comment at: include/llvm/Demangle/ItaniumDemangle.h:28
 
-namespace {
 // Base class of all AST nodes. The AST is built by the parser, then is
----------------
Don't remove this in the libcxxabi version, or enclose the `#include` in namespace {}!. You were probably planning on doing that, but just to be sure.


================
Comment at: lib/Demangle/ItaniumDemangle.cpp:63
 #ifndef NDEBUG
-  LLVM_DUMP_METHOD void dump() const {
-    char *Buffer = static_cast<char*>(std::malloc(1024));
-    OutputStream S(Buffer, 1024);
-    print(S);
-    S += '\0';
-    printf("Symbol dump for %p: %s\n", (const void*)this, S.getBuffer());
-    std::free(S.getBuffer());
-  }
-#endif
-};
-
-class NodeArray {
-  Node **Elements;
-  size_t NumElements;
-
-public:
-  NodeArray() : Elements(nullptr), NumElements(0) {}
-  NodeArray(Node **Elements_, size_t NumElements_)
-      : Elements(Elements_), NumElements(NumElements_) {}
-
-  bool empty() const { return NumElements == 0; }
-  size_t size() const { return NumElements; }
-
-  Node **begin() const { return Elements; }
-  Node **end() const { return Elements + NumElements; }
-
-  Node *operator[](size_t Idx) const { return Elements[Idx]; }
-
-  void printWithComma(OutputStream &S) const {
-    bool FirstElement = true;
-    for (size_t Idx = 0; Idx != NumElements; ++Idx) {
-      size_t BeforeComma = S.getCurrentPosition();
-      if (!FirstElement)
-        S += ", ";
-      size_t AfterComma = S.getCurrentPosition();
-      Elements[Idx]->print(S);
-
-      // Elements[Idx] is an empty parameter pack expansion, we should erase the
-      // comma we just printed.
-      if (AfterComma == S.getCurrentPosition()) {
-        S.setCurrentPosition(BeforeComma);
-        continue;
-      }
-
-      FirstElement = false;
-    }
-  }
-};
+struct DumpVisitor {
+  unsigned Depth = 0;
----------------
This should be defined in an anon namespace, right?


================
Comment at: lib/Demangle/ItaniumDemangle.cpp:236-238
+//===----------------------------------------------------------------------===//
+// Code beyond this point should not be synchronized with libc++abi.
+//===----------------------------------------------------------------------===//
----------------
I think this comment is too high: initializeOutputStream() and BumpPointerAllocator are both used in the libcxxabi version.


Repository:
  rL LLVM

https://reviews.llvm.org/D50930