[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);
----------------
njames93 wrote:
> 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.
Oh, interesting. How I hate `auto` as a code reviewer...


================
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.
----------------
njames93 wrote:
> 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.
This compounds the problem by making the AST matchers harder to write because now we're enshrining that there's two different APIs that do the same thing but you have to remember which one needs to be called in what mode and for which kinds of nodes.

"Go fix the AST nodes" is not a solution for this patch, but I think our long-term goal should be to try to add some uniformity at the AST node level so that we can eventually remove APIs like this and hopefully reduce the amount of places we're doing `isa<>` checks for specific node types to try to decide whether it was spelled in the source or not. Obviously, I'm not asking you to do that work -- just trying to see if we have some agreement on an aspirational goal.

Concretely, what do you think about an API like:
```
// FIXME: This is necessary only because there's no good general way to know 
// whether an AST node was spelled in source or not, so callers of matchesFirstInPointerRange
// need to decide whether they want to ignore implicit nodes or not. Ideally, we could use the
// Finder object only to make this decision, but we're not there quite yet.
enum class MatchFirstPointerInRangeMode {
  AsIs,
  IgnoreUnlessSpelledInSource
};

template <typename MatcherT, typename IteratorT>
IteratorT matchesFirstInPointerRange(const MatcherT &Matcher, IteratorT Start,
                                     IteratorT End, ASTMatchFinder *Finder,
                                     BoundNodesTreeBuilder *Builder,
                                     MatchFirstPointerInRangeMode Mode = MatchFirstPointerInRangeMode::AsIs);
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97246