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()))
----------------
LegalizeAdulthood wrote:
> 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.
Also, regarding a variadic version of this matcher, I'd be curious to try it out
just from a learning/programming perspective, but I'm not sure how I'd go about
it. Suggestions on a plan of attack would be most welcome! `:)`
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
