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

[Michael Buch via Phabricator via llvm-commits]

Michael137 added inline comments.


================
Comment at: llvm/include/llvm/Demangle/ItaniumDemangle.h:233
+  /// Behaviour is undefined if 'Root == nullptr'.
+  bool equals(Node const* Root) const;
+
----------------
ldionne wrote:
> 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.
That's a reasonable alternative. It does give us some more flexibility in comparing concrete vs. abstract nodes. With my implementation we'd have to cast a concrete node back to `Node*` to compare for equality which isn't very ergonomic.


================
Comment at: llvm/include/llvm/Demangle/ItaniumDemangle.h:372
+
+  friend bool operator==(NodeArrayNode const& LHS, NodeArrayNode const& RHS) {
+    return LHS.Array == RHS.Array;
----------------
ldionne wrote:
> 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)`.
Will do


================
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;
----------------
ldionne wrote:
> Just a suggestion, non-blocking. I find that clearer but it may just be preference.
I find your approach more readable :)
will do that instead, thanks


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131331