[PATCH] D131331: WIP: [llvm] lldb abi_tag support 1/3 - Add equality comparison API to itanium_demangle::Node

[Louis Dionne via Phabricator via llvm-commits]

ldionne added a comment.

Once you use the libcxxabi sources as a base, the `libcxxabi` review group should kick in and our CI should get triggered.



================
Comment at: llvm/include/llvm/Demangle/ItaniumDemangle.h:233
+  /// Behaviour is undefined if 'Root == nullptr'.
+  bool equals(Node const* Root) const;
+
----------------
I think I don't understand why there is both `operator==` and `equals`. Do we really need both?

Instead, you could consider defining a `virtual bool equals(Node const* Root) const = 0;` and override it in subclasses, which seems to be a bit more in-line with what's done elsewhere in this file (e.g. `printRight`). Thoughts? It does mean that all overrides are going to have to check that `Root->Kind == this->Kind`, but that may be acceptable.


================
Comment at: llvm/include/llvm/Demangle/ItaniumDemangle.h:372
+
+  friend bool operator==(NodeArrayNode const& LHS, NodeArrayNode const& RHS) {
+    return LHS.Array == RHS.Array;
----------------
If you keep defining `operator==` after my comment above, I feel like all classes that get an `operator==` should also get an `operator!=` defined with the usual `!(a == b)`.


================
Comment at: llvm/include/llvm/Demangle/ItaniumDemangle.h:435-440
+    if (!!LHS.TA != !!RHS.TA)
+      return false;
+
+    if (LHS.TA)
+      if (!LHS.TA->equals(RHS.TA))
+        return false;
----------------
Just a suggestion, non-blocking. I find that clearer but it may just be preference.


================
Comment at: llvm/unittests/Demangle/ItaniumDemangleTestUtils.h:1
+//===------------------ ItaniumDemangleTestUtils.h ------------------------===//
+//
----------------
I suspect `cp_to_llvm.sh` will have to be updated to know about this new file.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131331