[PATCH] D54407: Record the matcher type when storing a binding

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In https://reviews.llvm.org/D54407#1295555, @sbenza wrote:

> In https://reviews.llvm.org/D54407#1294934, @aaron.ballman wrote:
>
> > Adding @sbenza in case he has opinions on this approach. I think it's reasonable, but I also know that changes to the the AST matcher internals sometimes have unintended side effects with the various instantiations.
>
>
> The problem used to come with the number of template instantiations, but afaik this was with an MSVC configuration that is no longer supported.
>  In any case, I don't see this change making new template instantiations.


Ah, I think I was remembering the issues from binary sizes due to template instantiations. I didn't think this would introduce new instantiations, but I thought it might make them all larger in a harmful way. I don't think that's a concern here, but your second set of eyes doesn't hurt.



================
Comment at: include/clang/ASTMatchers/ASTMatchersInternal.h:153
+
+    bool operator<(const NodeEntry &other) const {
+      return DynNode < other.DynNode && NodeKind < other.NodeKind;
----------------
other -> Other


================
Comment at: include/clang/ASTMatchers/ASTMatchersInternal.h:154
+    bool operator<(const NodeEntry &other) const {
+      return DynNode < other.DynNode && NodeKind < other.NodeKind;
+    }
----------------
This doesn't provide the right ordering guarantees. Use `std::tie()` instead: `return std::tie(DynNode, NodeKind) < std::tie(Other.DynNode, Other.NodeKind);`


Repository:
  rC Clang

https://reviews.llvm.org/D54407