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

[Stephen Kelly via Phabricator via cfe-commits]

steveire added inline comments.


================
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);
----------------
aaron.ballman wrote:
> Since you're touching the line anyway, might as well fix the lint warning and use `const auto *`.
This is iteration from a `begin()` to an `end()`. It's appropriate to use `auto` for an iterator type instead of `const auto *`.

You don't have to change it back now, but the request to change it in the first place isn't consistent. I also don't think it's ever appropriate to use `const auto *` or `auto *` instead of just `auto`, so make of that what you wish :).

Anyway, to completely dodge the issue you can inline it and get rid of the `MatchIt` altogether without changing the meaning of the code.


================
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:
> 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);
> ```
> ```
> enum class MatchFirstPointerInRangeMode {
>   AsIs,
>   IgnoreUnlessSpelledInSource
> };
> ```

Why introduce a new enum instead of just using `clang::TraversalKind`?



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97246