[PATCH] D61837: Make it possible control matcher traversal kind with ASTContext
Aaron Ballman via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Fri May 17 06:23:36 PDT 2019
aaron.ballman added inline comments.
================
Comment at: lib/AST/ASTContext.cpp:120
+ASTContext::TraverseIgnored(const ast_type_traits::DynTypedNode &N) {
+ if (auto E = N.get<Expr>()) {
+ return ast_type_traits::DynTypedNode::create(*TraverseIgnored(E));
----------------
aaron.ballman wrote:
> `auto *`
The formatting is wrong here -- be sure to run the patch through clang-format before committing.
================
Comment at: lib/ASTMatchers/ASTMatchersInternal.cpp:240
+
+ assert(RestrictKind.isBaseOf(NodeKind));
+ if (Implementation->dynMatches(N, Finder, Builder)) {
----------------
steveire wrote:
> aaron.ballman wrote:
> > Add an assertion message?
> Saying what? The original code doesn't have one. Let's avoid round trips in review comments :).
This isn't a "round trip"; it's not unreasonable to ask people to NFC improve the code they're touching (it's akin to saying "Because you figured out this complex piece of code does X, can you add a comment to it so others don't have to do that work next time.").
As best I can tell, this assertion exists because this function is meant to mirror `matches()` without this base check in release mode. You've lost that mirroring with your refactoring, which looks suspicious. Is there a reason this function deviates from `matches()` now?
As for the assertion message itself, how about "matched a node of an unexpected derived kind"?
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D61837/new/
https://reviews.llvm.org/D61837
More information about the cfe-commits
mailing list