LegalizeAdulthood added inline comments.
================
Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:5435-5442
+/// Matches two consecutive statements within a compound statement.
+///
+/// Given
+/// \code
+/// { if (x > 0) return true; return false; }
+/// \endcode
+/// compoundStmt(hasSubstatementSequence(ifStmt(), returnStmt()))
----------------
aaron.ballman wrote:
> LegalizeAdulthood wrote:
> > aaron.ballman wrote:
> > > LegalizeAdulthood wrote:
> > > > aaron.ballman wrote:
> > > > > How do we extend this to support testing arbitrary sequences of
> > > > > statements? (If this supports two statements, someone will find a
> > > > > need for three, etc).
> > > > Yeah, I was wondering that too. I didn't see (but didn't look
> > > > extensively) any support for variadic matchers taking a parameter pack.
> > > >
> > > > I stopped at 2 because this suits my immediate needs with
> > > > `readability-simplify-boolean-expr` where I have to manually loop over
> > > > `CompoundStmt` matches in order to verify that the `if (x) return true;
> > > > return false;` constructs consist of two adjacent statements.
> > > I don't think we have any variadic matchers yet to model on, but I think
> > > if this is only needed for one check, we can add it in the current form
> > > as a local matcher for that check. Whenever we figure out how to give the
> > > better interface through the static and dynamic matchers, then we can
> > > figure out how to lift this to the general matcher interface.
> > >
> > > WDYT?
> > I don't think it is harmful to make it visible to all and I think it is
> > helpful.
> > Defining it in ASTMatchers, enables using it in `clang-query`, for instance.
> >
> I contend this is not a generally useful matcher without supporting an
> arbitrary number of statements. Even then, to be honest, it's questionable
> whether there's sufficient need for this to be a public matcher. Typically,
> we don't expose a new public matcher unless there's a general need for it,
> and this one is already pretty borderline even if it's fully generalized.
> This file is *very* expensive to instantiate and it gets used in a lot of
> places, so that's one of the primary reasons we don't expose matchers from
> here unless they're generally useful.
>
> Unless @klimek or another AST matcher code owner thinks this is useful in
> general (to multiple checks people are likely to want to write even if
> they're not contributing the checks to the community), I'm opposed to
> exposing this as-is. However, adding it as a private matcher for the check
> that needs the limited functionality would get no opposition from me (and
> this implementation looks correct as well).
My thoughts:
I'm OK with moving it as a private matcher as it would simplify a big chunk of
code
in the simplify-boolean-expr check.
I agree that ASTMatchers.h is used all over the place and at a minimum causes a
huge amount of rebuild.
Regarding the general usefulness of the matcher, let me elaborate on my
motivation
for adding this matcher.
I see it from the viewpoint of a developer of checks/refactorings.
I really want us to get to a world where a complete refactoring can be
specified as
a script input to a refactoring tool. Eliminating the need to write C++ that
directly
manipulates the AST and the edits will lower the bar for entry for other people
writing automated changes to their codebases.
Since the last time I worked in the clang codebase, the transformer library has
been added. With this new library, I think all we're missing is a parser that
matches
the input script to the necessary calls to code in the transformer library.
Once we
do this, I think the need for more and higher-level matchers will become evident
and a matcher like this is the only way you can specify that a statement Y
immediately follows statement X purely through matchers. Current matchers
don't even let you specify relative ordering of statements. The best you can do
is assert that a block contains the statements of interest and then you must
write
your own C++ code to walk the block and determine if they fit your actual
match criteria.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D116518/new/
https://reviews.llvm.org/D116518
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
