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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
