aaron.ballman 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:
> > > > 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).


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

Reply via email to