[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
llvm-commits at lists.llvm.org
Wed Aug 10 11:38:16 PDT 2022
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
More information about the llvm-commits
mailing list