[PATCH] D97246: [ASTMatchers] Fix matching failure in IgnoreUnlessSpelledInSource mode

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:3296
   BoundNodesTreeBuilder Result(*Builder);
-  auto MatchIt = matchesFirstInPointerRange(InnerMatcher, Node.method_begin(),
-                                            Node.method_end(), Finder, &Result);
+  auto MatchIt = matchesFirstInPointerRangeIgnoreUnspelled(
+      InnerMatcher, Node.method_begin(), Node.method_end(), Finder, &Result);
----------------
It amuses me that this lack of `const auto *` was not similarly flagged as the one below, but feel free to correct this one as well.


================
Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:4358
               internal::Matcher<CXXCtorInitializer>, InnerMatcher) {
-  auto MatchIt = matchesFirstInPointerRange(InnerMatcher, Node.init_begin(),
-                                            Node.init_end(), Finder, Builder);
-  if (MatchIt == Node.init_end())
-    return false;
-  return (*MatchIt)->isWritten() || !Finder->isTraversalIgnoringImplicitNodes();
+  auto MatchIt = matchesFirstInPointerRangeIgnoreUnspelled(
+      InnerMatcher, Node.init_begin(), Node.init_end(), Finder, Builder);
----------------
Since you're touching the line anyway, might as well fix the lint warning and use `const auto *`.


================
Comment at: clang/include/clang/ASTMatchers/ASTMatchersInternal.h:817-819
+/// Finds the first node in a pointer range that matches the given
+/// matcher. Ignores any nodes that aren't spelled in source if Finder is ignore
+/// them.
----------------
Comment looks like it can be re-flowed to 80 cols.

"if Finder is ignore them" -> "if Finder is ignoring them"

This comment is a bit odd because I would expect the same behavior from `matchesFirstInPointerRange` -- e.g., if the finder says "ignore unless spelled in source", I would not expect to have to call a function that also says to honor that, so this feels a bit fragile. I was sort of thinking that an (optional?) parameter to `matchesFirstInPointerPair` would be slightly better (and reduce code duplication), but that feels similarly fragile.

Would the logic be wrong to always honor what the `Finder` says about ignoring implicit nodes?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97246