[PATCH] D116328: [ast-matchers] Add hasSubstatement() traversal matcher for caseStmt(), defaultStmt(), labelStmt()

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood abandoned this revision.
LegalizeAdulthood added a comment.

Abandoning this in favor of a private matcher where it is needed.


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

https://reviews.llvm.org/D116328

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D116328: [ast-matchers] Add hasSubstatement() traversal matcher for caseStmt(), defaultStmt(), labelStmt()

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D116328#3241486 , 
@LegalizeAdulthood wrote:

> In D116328#3241329 , @aaron.ballman 
> wrote:
>
>> In D116328#3223344 , 
>> @LegalizeAdulthood wrote:
>>
>>> My takeaway:
>>>
>>> - if `has` isn't expensive, I can either ditch this public matcher or move 
>>> it to be a private matcher used in my check
>>
>> [...] if you profiled something and notice a measurable difference
>> between `has()` and `hasSubstatement()` in practice, that would
>> be really good for the community to know.
>
> Do we have some sort of benchmarking facility in place, or do I
> have to homebrew something?

I've always homebrewed it, but if we have better facilities these days, I'd 
love to know about them!


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

https://reviews.llvm.org/D116328

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D116328: [ast-matchers] Add hasSubstatement() traversal matcher for caseStmt(), defaultStmt(), labelStmt()

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added a comment.

In D116328#3241329 , @aaron.ballman 
wrote:

> In D116328#3223344 , 
> @LegalizeAdulthood wrote:
>
>> My takeaway:
>>
>> - if `has` isn't expensive, I can either ditch this public matcher or move 
>> it to be a private matcher used in my check
>
> [...] if you profiled something and notice a measurable difference
> between `has()` and `hasSubstatement()` in practice, that would
> be really good for the community to know.

Do we have some sort of benchmarking facility in place, or do I
have to homebrew something?


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

https://reviews.llvm.org/D116328

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D116328: [ast-matchers] Add hasSubstatement() traversal matcher for caseStmt(), defaultStmt(), labelStmt()

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D116328#3223344 , 
@LegalizeAdulthood wrote:

> My takeaway:
>
> - if `has` isn't expensive, I can either ditch this public matcher or move it 
> to be a private matcher used in my check

I don't think `has()` is particularly expensive, so I think you should be able 
to use it directly. However, if you profiled something and notice a measurable 
difference between `has()` and `hasSubstatement()` in practice, that would be 
really good for the community to know.


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

https://reviews.llvm.org/D116328

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D116328: [ast-matchers] Add hasSubstatement() traversal matcher for caseStmt(), defaultStmt(), labelStmt()

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added a comment.

Ping.

This review has been waiting for a week without any comment.


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

https://reviews.llvm.org/D116328

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D116328: [ast-matchers] Add hasSubstatement() traversal matcher for caseStmt(), defaultStmt(), labelStmt()

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added a comment.

In D116328#3223299 , @aaron.ballman 
wrote:

> In D116328#3223268 , 
> @LegalizeAdulthood wrote:
>
>> In D116328#3221995 , 
>> @aaron.ballman wrote:
>>
>>> 
>
> `has()` matches the direct descendant AST node, whereas `hasDescendant()` 
> matches *any* descendant AST node.
> [...] I don't know of any performance concerns with `has()` though.

Thanks for the explanation!

>> Honestly, at this point, I become lost in manually tracing the code through 
>> "go to definition" in my IDE.
>
> I'm impressed you got this far -- the AST matching machinery under the hood 
> is really quite complex! :-)

Having CMake generate a VS project and drilling in with ReSharper for C++ is
pretty powerful :), but you have to manually keep track of how the template
and function arguments are being resolved in the dynamic execution.  So it's
pretty easy to get lost in template oriented code, which is understandably
pretty common in AST land.

> My understanding (and my recollection here may be wrong) is that `has()` can 
> memoize the results
> [...] but you can't get the same behavior from `hasDescendant()` or 
> `hasAncestor()`

OK, that makes sense.

> I'm adding @sammccall in case he has information about the performance 
> characteristics or thoughts about
> the proposed matcher in general.

So let me summarize what we've learned so far:

- `has` only descends one level, is memoizable, and is less expensive than 
`hasDescendant`
- `has` uses the Visitor pattern to compute the result that is memoized
- @sammccall  might be aware of any performance related issues with `has`

My takeaway:

- if `has` isn't expensive, I can either ditch this public matcher or move it 
to be a private matcher used in my check


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

https://reviews.llvm.org/D116328

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D116328: [ast-matchers] Add hasSubstatement() traversal matcher for caseStmt(), defaultStmt(), labelStmt()

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a reviewer: sammccall.
aaron.ballman added a subscriber: sammccall.
aaron.ballman added a comment.

In D116328#3223268 , 
@LegalizeAdulthood wrote:

> In D116328#3221995 , @aaron.ballman 
> wrote:
>
>>> Previously if you wanted to match the statement associated with a case, 
>>> default, or labelled statement, you had to use hasDescendant.
>>
>> This isn't true. You can use `has()` to do direct substatemematching, and 
>> I'm wondering whether we need this new matcher at all. e.g., 
>> https://godbolt.org/z/K5sYP757r
>
> Well, the whole point of this was that previous review comments were 
> complaining that I was using an "expensive" matcher
> instead of one that got right to the point.  (What's the difference between 
> `has` and `hasDescendent` anyway?)

`has()` matches the direct descendant AST node, whereas `hasDescendant()` 
matches *any* descendant AST node. So `hasDescendant()` actually is expensive 
-- it can traverse the AST more times than you'd expect. I don't know of any 
performance concerns with `has()` though.

e.g., https://godbolt.org/z/n3adEz4qW (the first query matches nothing, the 
second query has a match)

> I looked at the implementation of "has" and it has a huge amount of machinery 
> underneath it.  I drilled in as follows (my ToT hash is 
> 773ab3c6655f4d2beec25bb3516b4d4fe2eea990 
> ):
> ASTMatchers.h:3391 declares `has` as an instance of 
> `internal::ArgumentAdaptingMatcherFunc`
> `ArgumentAdaptingMatcherFun` appears just to be a factory mechanism for 
> creating the appropriate matcher.
> `HasMatcher` seems to do the actual matching
> `HasMatcher::matches` delegates to 
> `ASTMatchFinder::matchesChildOf(const T , ...)`
> `ASTMatchFinder::matchesChildOf(const T , ...)` delegates to another 
> overload of `matchesChildOf`(const DynTypedNode , ...)` after asserting 
> a static type relationship (ASTMatchersInternal.h:762)
> `ASTMatchFinder::matchesChildOf(const DynTypedNode , ...)` is a virtual 
> function with one implementation in `MatchASTVisitor` (ASTMatchFinder.cpp:659)
> `MatchASTVisitor::matchesChildOf` delegates to `memoizedMatchesRecursively` 
> (ASTMatchFinder.cpp:664)
> For non-memoizable nodes, `MatchASTVisitor::memoizedMatchesRecursively` 
> delegates to `matchesRecursively` (ASTMatchFinder.cpp:599)
> `MatchASTVIsitor::matchesRecursively` instantiates a 
> `ASTNodeNotSpelledInSourceScope` and a `MatchChildASTVisitor` upon which it 
> calls `findMatch` (ASTMatchFinder.cpp:645)
> `MatchChildASTVisitor::findMatch` does a series of `DynNode.get` checks 
> and delegates to the appropriate overload of 
> `MatchChildASTVisitor::traverse`, in our case it would be the one for `Stmt`
> `MatchChildASTVisitor::traverse(const T )` delegates to 
> `MatchChildASTVisitor::match(const T )`
> `MatchChildASTVisitor::match(const T )` instantiates a 
> `BoundNodesTreeBuilder` and calls `match` on the held `Matcher`.
>
> Honestly, at this point, I become lost in manually tracing the code through 
> "go to definition" in my IDE.

I'm impressed you got this far -- the AST matching machinery under the hood is 
really quite complex! :-)

> So I switched my unit test for `HasCaseSubstmt` to use `has` and drilled in 
> through the debugger.
> What I saw echoes the above, although I took the wrong branch in my manual 
> analysis in `memoizedMatchesRecursively`, it goes down the memoization path.

Phew, that's good -- I would expect `has` to use the memoization pass.

> However, the whole point of memoizing a result is because that result was 
> expensive to compute and the memoization saves time on subsequent queries
> because the result has been memorized.  In this case, it's a simple getter on 
> `CaseStmt` to obtain the node against which you want to match, so I don't
> see the memoization as having any particular benefit.  (The inner matcher to 
> `hasSubstatement` may be expensive and could be memoized.)  For results
> obtain by the visitor pattern, I can see why you'd want to memoize them as 
> there is lots of code being executed to implement the Visitor pattern.

My understanding (and my recollection here may be wrong) is that `has()` can 
memoize the results because the sub-matcher will always be matched against the 
immediate direct descendant AST node, but you can't get the same behavior from 
`hasDescendant()` or `hasAncestor()` because any of the descendant (or 
ancestor) nodes can be matched, so you have to traverse them instead of 
memoizing. I'm adding @sammccall in case he has information about the 
performance characteristics or thoughts about the proposed matcher in general.


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

https://reviews.llvm.org/D116328

___
cfe-commits mailing list
cfe-commits@lists.llvm.org

[PATCH] D116328: [ast-matchers] Add hasSubstatement() traversal matcher for caseStmt(), defaultStmt(), labelStmt()

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood marked an inline comment as done.
LegalizeAdulthood added a comment.

In D116328#3221995 , @aaron.ballman 
wrote:

>> Previously if you wanted to match the statement associated with a case, 
>> default, or labelled statement, you had to use hasDescendant.
>
> This isn't true. You can use `has()` to do direct substatemematching, and I'm 
> wondering whether we need this new matcher at all. e.g., 
> https://godbolt.org/z/K5sYP757r

Well, the whole point of this was that previous review comments were 
complaining that I was using an "expensive" matcher
instead of one that got right to the point.  (What's the difference between 
`has` and `hasDescendent` anyway?)

I looked at the implementation of "has" and it has a huge amount of machinery 
underneath it.  I drilled in as follows (my ToT hash is 
773ab3c6655f4d2beec25bb3516b4d4fe2eea990 
):
ASTMatchers.h:3391 declares `has` as an instance of 
`internal::ArgumentAdaptingMatcherFunc`
`ArgumentAdaptingMatcherFun` appears just to be a factory mechanism for 
creating the appropriate matcher.
`HasMatcher` seems to do the actual matching
`HasMatcher::matches` delegates to 
`ASTMatchFinder::matchesChildOf(const T , ...)`
`ASTMatchFinder::matchesChildOf(const T , ...)` delegates to another 
overload of `matchesChildOf`(const DynTypedNode , ...)` after asserting a 
static type relationship (ASTMatchersInternal.h:762)
`ASTMatchFinder::matchesChildOf(const DynTypedNode , ...)` is a virtual 
function with one implementation in `MatchASTVisitor` (ASTMatchFinder.cpp:659)
`MatchASTVisitor::matchesChildOf` delegates to `memoizedMatchesRecursively` 
(ASTMatchFinder.cpp:664)
For non-memoizable nodes, `MatchASTVisitor::memoizedMatchesRecursively` 
delegates to `matchesRecursively` (ASTMatchFinder.cpp:599)

  `MatchASTVIsitor::matchesRecursively` instantiates a 
`ASTNodeNotSpelledInSourceScope` and a `MatchChildASTVisitor` upon which it 
calls `findMatch` (ASTMatchFinder.cpp:645)
  `MatchChildASTVisitor::findMatch` does a series of `DynNode.get` checks 
and delegates to the appropriate overload of `MatchChildASTVisitor::traverse`, 
in our case it would be the one for `Stmt`
  `MatchChildASTVisitor::traverse(const T )` delegates to 
`MatchChildASTVisitor::match(const T )`
  `MatchChildASTVisitor::match(const T )` instantiates a 
`BoundNodesTreeBuilder` and calls `match` on the held `Matcher`.

Honestly, at this point, I become lost in manually tracing the code through "go 
to definition" in my IDE.  So I switched my unit test for `HasCaseSubstmt` to 
use `has` and drilled in through the debugger.
What I saw echoes the above, although I took the wrong branch in my manual 
analysis in `memoizedMatchesRecursively`, it goes down the memoization path.

However, the whole point of memoizing a result is because that result was 
expensive to compute and the memoization saves time on subsequent queries
because the result has been memorized.  In this case, it's a simple getter on 
`CaseStmt` to obtain the node against which you want to match, so I don't
see the memoization as having any particular benefit.  (The inner matcher to 
`hasSubstatement` may be expensive and could be memoized.)  For results
obtain by the visitor pattern, I can see why you'd want to memoize them as 
there is lots of code being executed to implement the Visitor pattern.




Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:2432
 dependentCoawaitExpr;
+
 /// Matches co_yield expressions.

aaron.ballman wrote:
> Spurious newline?
I didn't add this intentionally and I can remove it, but the general style in 
this file is to separate top-level decls by a blank line.


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

https://reviews.llvm.org/D116328

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D116328: [ast-matchers] Add hasSubstatement() traversal matcher for caseStmt(), defaultStmt(), labelStmt()

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

> Previously if you wanted to match the statement associated with a case, 
> default, or labelled statement, you had to use hasDescendant.

This isn't true. You can use `has()` to do direct substatemematching, and I'm 
wondering whether we need this new matcher at all. e.g., 
https://godbolt.org/z/K5sYP757r




Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:2432
 dependentCoawaitExpr;
+
 /// Matches co_yield expressions.

Spurious newline?



Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:7720
 
+/// Matches the substatement associated with a case, default or label 
statement.
+///

May need to reformat as well, but the intent is to make it clear that this does 
not behave like `hasAnySubstatement()`.



Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:7727
+///   foo: return;
+///   bar: break;
+/// \endcode

This is invalid code (there's nothing to break out of), so you may want to 
tweak the example a bit.



Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:7730-7734
+/// caseStmt(hasSubstmt(returnStmt()))
+///   matches "case 2: return;"
+/// defaultStmt(hasSubstmt(returnStmt()))
+///   matches "default: return;"
+/// labelStmt(hasSubstmt(breakStmt()))





Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:7737-7738
+AST_POLYMORPHIC_MATCHER_P(hasSubstatement,
+  AST_POLYMORPHIC_SUPPORTED_TYPES(CaseStmt, 
DefaultStmt,
+  LabelStmt),
+  internal::Matcher, InnerMatcher) {

Pretty sure you can use the base class here as a simplification.



Comment at: clang/lib/ASTMatchers/Dynamic/Registry.cpp:69
   registerMatcher(#name, internal::makeMatcherAutoMarshall(
\
- ::clang::ast_matchers::name, #name));
+ ::clang::ast_matchers::name, #name))
 

Good catch; feel free to land this and related changes as an NFC commit.



Comment at: clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp:161
+TEST_P(ASTMatchersTest, HasLabelSubstmt) {
+  EXPECT_TRUE(matches("void f() { while (1) { bar: break; foo: return; } }",
+  traverse(TK_AsIs, 
labelStmt(hasSubstatement(breakStmt());

Should fix the formatting.


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

https://reviews.llvm.org/D116328

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D116328: [ast-matchers] Add hasSubstatement() traversal matcher for caseStmt(), defaultStmt(), labelStmt()

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood updated this revision to Diff 396933.
LegalizeAdulthood added a comment.

Document the new matcher in the clang release notes


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

https://reviews.llvm.org/D116328

Files:
  clang/docs/LibASTMatchersReference.html
  clang/docs/ReleaseNotes.rst
  clang/include/clang/ASTMatchers/ASTMatchers.h
  clang/lib/ASTMatchers/Dynamic/Registry.cpp
  clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp

Index: clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
===
--- clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
+++ clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
@@ -145,6 +145,23 @@
 HasDescendantVariableI));
 }
 
+TEST_P(ASTMatchersTest, HasCaseSubstmt) {
+  EXPECT_TRUE(matches(
+  "void f() { switch (1) { case 1: return; break; default: break; } }",
+  traverse(TK_AsIs, caseStmt(hasSubstatement(returnStmt());
+}
+
+TEST_P(ASTMatchersTest, HasDefaultSubstmt) {
+  EXPECT_TRUE(matches(
+  "void f() { switch (1) { case 1: return; break; default: break; } }",
+  traverse(TK_AsIs, defaultStmt(hasSubstatement(breakStmt());
+}
+
+TEST_P(ASTMatchersTest, HasLabelSubstmt) {
+  EXPECT_TRUE(matches("void f() { while (1) { bar: break; foo: return; } }",
+  traverse(TK_AsIs, labelStmt(hasSubstatement(breakStmt());
+}
+
 TEST(TypeMatcher, MatchesClassType) {
   TypeMatcher TypeA = hasDeclaration(recordDecl(hasName("A")));
 
Index: clang/lib/ASTMatchers/Dynamic/Registry.cpp
===
--- clang/lib/ASTMatchers/Dynamic/Registry.cpp
+++ clang/lib/ASTMatchers/Dynamic/Registry.cpp
@@ -66,7 +66,7 @@
 
 #define REGISTER_MATCHER(name) \
   registerMatcher(#name, internal::makeMatcherAutoMarshall(\
- ::clang::ast_matchers::name, #name));
+ ::clang::ast_matchers::name, #name))
 
 #define REGISTER_MATCHER_OVERLOAD(name)\
   registerMatcher(#name,   \
@@ -143,7 +143,7 @@
   REGISTER_MATCHER(atomicType);
   REGISTER_MATCHER(attr);
   REGISTER_MATCHER(autoType);
-  REGISTER_MATCHER(autoreleasePoolStmt)
+  REGISTER_MATCHER(autoreleasePoolStmt);
   REGISTER_MATCHER(binaryConditionalOperator);
   REGISTER_MATCHER(binaryOperator);
   REGISTER_MATCHER(binaryOperation);
@@ -355,6 +355,7 @@
   REGISTER_MATCHER(hasSpecializedTemplate);
   REGISTER_MATCHER(hasStaticStorageDuration);
   REGISTER_MATCHER(hasStructuredBlock);
+  REGISTER_MATCHER(hasSubstatement);
   REGISTER_MATCHER(hasSyntacticForm);
   REGISTER_MATCHER(hasTargetDecl);
   REGISTER_MATCHER(hasTemplateArgument);
Index: clang/include/clang/ASTMatchers/ASTMatchers.h
===
--- clang/include/clang/ASTMatchers/ASTMatchers.h
+++ clang/include/clang/ASTMatchers/ASTMatchers.h
@@ -2429,6 +2429,7 @@
 /// Matches co_await expressions where the type of the promise is dependent
 extern const internal::VariadicDynCastAllOfMatcher
 dependentCoawaitExpr;
+
 /// Matches co_yield expressions.
 ///
 /// Given
@@ -7716,6 +7717,29 @@
   return InnerMatcher.matches(*Node.getLHS(), Finder, Builder);
 }
 
+/// Matches the substatement associated with a case, default or label statement.
+///
+/// Given
+/// \code
+///   switch (1) { case 1: break; case 2: return; break; default: return; break;
+///   }
+///   foo: return;
+///   bar: break;
+/// \endcode
+///
+/// caseStmt(hasSubstmt(returnStmt()))
+///   matches "case 2: return;"
+/// defaultStmt(hasSubstmt(returnStmt()))
+///   matches "default: return;"
+/// labelStmt(hasSubstmt(breakStmt()))
+///   matches "bar: break;"
+AST_POLYMORPHIC_MATCHER_P(hasSubstatement,
+  AST_POLYMORPHIC_SUPPORTED_TYPES(CaseStmt, DefaultStmt,
+  LabelStmt),
+  internal::Matcher, InnerMatcher) {
+  return InnerMatcher.matches(*Node.getSubStmt(), Finder, Builder);
+}
+
 /// Matches declaration that has a given attribute.
 ///
 /// Given
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -277,6 +277,9 @@
   and the underlying ``Type`` with ``hasUnderlyingType``.
   ``hasDeclaration`` continues to see through the alias and apply to the
   underlying type.
+- The ``hasSubstatement`` matcher is now available and allows you to match the
+  statement associated with a labelled construct: case statement, default
+  statement or label statement.
 
 clang-format
 
Index: clang/docs/LibASTMatchersReference.html
===
--- 

[PATCH] D116328: [ast-matchers] Add hasSubstatement() traversal matcher for caseStmt(), defaultStmt(), labelStmt()

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood updated this revision to Diff 396878.
LegalizeAdulthood retitled this revision from "[ast-matchers] Add hasSubstmt() 
traversal matcher for caseStmt(), defaultStmt(), labelStmt()" to 
"[ast-matchers] Add hasSubstatement() traversal matcher for caseStmt(), 
defaultStmt(), labelStmt()".
LegalizeAdulthood added a comment.

Rename hasSubstmt to hasSubstatement


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

https://reviews.llvm.org/D116328

Files:
  clang/docs/LibASTMatchersReference.html
  clang/include/clang/ASTMatchers/ASTMatchers.h
  clang/lib/ASTMatchers/Dynamic/Registry.cpp
  clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp

Index: clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
===
--- clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
+++ clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
@@ -145,6 +145,23 @@
 HasDescendantVariableI));
 }
 
+TEST_P(ASTMatchersTest, HasCaseSubstmt) {
+  EXPECT_TRUE(matches(
+  "void f() { switch (1) { case 1: return; break; default: break; } }",
+  traverse(TK_AsIs, caseStmt(hasSubstatement(returnStmt());
+}
+
+TEST_P(ASTMatchersTest, HasDefaultSubstmt) {
+  EXPECT_TRUE(matches(
+  "void f() { switch (1) { case 1: return; break; default: break; } }",
+  traverse(TK_AsIs, defaultStmt(hasSubstatement(breakStmt());
+}
+
+TEST_P(ASTMatchersTest, HasLabelSubstmt) {
+  EXPECT_TRUE(matches("void f() { while (1) { bar: break; foo: return; } }",
+  traverse(TK_AsIs, labelStmt(hasSubstatement(breakStmt());
+}
+
 TEST(TypeMatcher, MatchesClassType) {
   TypeMatcher TypeA = hasDeclaration(recordDecl(hasName("A")));
 
Index: clang/lib/ASTMatchers/Dynamic/Registry.cpp
===
--- clang/lib/ASTMatchers/Dynamic/Registry.cpp
+++ clang/lib/ASTMatchers/Dynamic/Registry.cpp
@@ -66,7 +66,7 @@
 
 #define REGISTER_MATCHER(name) \
   registerMatcher(#name, internal::makeMatcherAutoMarshall(\
- ::clang::ast_matchers::name, #name));
+ ::clang::ast_matchers::name, #name))
 
 #define REGISTER_MATCHER_OVERLOAD(name)\
   registerMatcher(#name,   \
@@ -143,7 +143,7 @@
   REGISTER_MATCHER(atomicType);
   REGISTER_MATCHER(attr);
   REGISTER_MATCHER(autoType);
-  REGISTER_MATCHER(autoreleasePoolStmt)
+  REGISTER_MATCHER(autoreleasePoolStmt);
   REGISTER_MATCHER(binaryConditionalOperator);
   REGISTER_MATCHER(binaryOperator);
   REGISTER_MATCHER(binaryOperation);
@@ -355,6 +355,7 @@
   REGISTER_MATCHER(hasSpecializedTemplate);
   REGISTER_MATCHER(hasStaticStorageDuration);
   REGISTER_MATCHER(hasStructuredBlock);
+  REGISTER_MATCHER(hasSubstatement);
   REGISTER_MATCHER(hasSyntacticForm);
   REGISTER_MATCHER(hasTargetDecl);
   REGISTER_MATCHER(hasTemplateArgument);
Index: clang/include/clang/ASTMatchers/ASTMatchers.h
===
--- clang/include/clang/ASTMatchers/ASTMatchers.h
+++ clang/include/clang/ASTMatchers/ASTMatchers.h
@@ -2429,6 +2429,7 @@
 /// Matches co_await expressions where the type of the promise is dependent
 extern const internal::VariadicDynCastAllOfMatcher
 dependentCoawaitExpr;
+
 /// Matches co_yield expressions.
 ///
 /// Given
@@ -7716,6 +7717,29 @@
   return InnerMatcher.matches(*Node.getLHS(), Finder, Builder);
 }
 
+/// Matches the substatement associated with a case, default or label statement.
+///
+/// Given
+/// \code
+///   switch (1) { case 1: break; case 2: return; break; default: return; break;
+///   }
+///   foo: return;
+///   bar: break;
+/// \endcode
+///
+/// caseStmt(hasSubstmt(returnStmt()))
+///   matches "case 2: return;"
+/// defaultStmt(hasSubstmt(returnStmt()))
+///   matches "default: return;"
+/// labelStmt(hasSubstmt(breakStmt()))
+///   matches "bar: break;"
+AST_POLYMORPHIC_MATCHER_P(hasSubstatement,
+  AST_POLYMORPHIC_SUPPORTED_TYPES(CaseStmt, DefaultStmt,
+  LabelStmt),
+  internal::Matcher, InnerMatcher) {
+  return InnerMatcher.matches(*Node.getSubStmt(), Finder, Builder);
+}
+
 /// Matches declaration that has a given attribute.
 ///
 /// Given
Index: clang/docs/LibASTMatchersReference.html
===
--- clang/docs/LibASTMatchersReference.html
+++ clang/docs/LibASTMatchersReference.html
@@ -7298,6 +7298,16 @@
 
 
 
+Matcherhttps://clang.llvm.org/doxygen/classclang_1_1CaseStmt.html;>CaseStmthasSubstatementMatcherhttps://clang.llvm.org/doxygen/classclang_1_1Stmt.html;>Stmt InnerMatcher
+Matches the substatement associated with a case statement.
+
+Given
+  switch (1) { case 1: