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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
