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

[Nathan James via Phabricator via cfe-commits]

njames93 marked 2 inline comments as done.
njames93 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);
----------------
aaron.ballman wrote:
> 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.
The reason it isn't flagged is because MatchIt is of type `specific_decl_iterator`, which isn't a pointer.


================
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.
----------------
aaron.ballman wrote:
> 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?
The issue is not all nodes have a nice way to say they are implicit and should be ignored.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97246