[PATCH] D81552: [ASTMatchers] Added hasDirectBase and hasClass Matchers

[Nathan James via Phabricator via cfe-commits]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rGb0d3ea171bd5: [ASTMatchers] Added hasDirectBase Matcher 
(authored by njames93).

Changed prior to commit:
  https://reviews.llvm.org/D81552?vs=27&id=275666#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81552

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

Index: clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
===
--- clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
+++ clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
@@ -3125,5 +3125,44 @@
  cxxRecordDecl(hasAnyBase(isVirtual();
 }
 
+TEST(BaseSpecifier, hasDirectBase) {
+  EXPECT_TRUE(matches(
+  R"cc(
+class Base {};
+class Derived : Base{};
+)cc",
+  cxxRecordDecl(hasName("Derived"),
+hasDirectBase(hasType(cxxRecordDecl(hasName("Base")));
+
+  StringRef MultiDerived = R"cc(
+class Base {};
+class Base2 {};
+class Derived : Base, Base2{};
+)cc";
+
+  EXPECT_TRUE(matches(
+  MultiDerived,
+  cxxRecordDecl(hasName("Derived"),
+hasDirectBase(hasType(cxxRecordDecl(hasName("Base")));
+  EXPECT_TRUE(matches(
+  MultiDerived,
+  cxxRecordDecl(hasName("Derived"),
+hasDirectBase(hasType(cxxRecordDecl(hasName("Base2")));
+
+  StringRef Indirect = R"cc(
+class Base {};
+class Intermediate : Base {};
+class Derived : Intermediate{};
+)cc";
+
+  EXPECT_TRUE(
+  matches(Indirect, cxxRecordDecl(hasName("Derived"),
+  hasDirectBase(hasType(cxxRecordDecl(
+  hasName("Intermediate")));
+  EXPECT_TRUE(notMatches(
+  Indirect,
+  cxxRecordDecl(hasName("Derived"),
+hasDirectBase(hasType(cxxRecordDecl(hasName("Base")));
+}
 } // namespace ast_matchers
 } // namespace clang
Index: clang/lib/ASTMatchers/Dynamic/Registry.cpp
===
--- clang/lib/ASTMatchers/Dynamic/Registry.cpp
+++ clang/lib/ASTMatchers/Dynamic/Registry.cpp
@@ -278,6 +278,7 @@
   REGISTER_MATCHER(hasDefinition);
   REGISTER_MATCHER(hasDescendant);
   REGISTER_MATCHER(hasDestinationType);
+  REGISTER_MATCHER(hasDirectBase);
   REGISTER_MATCHER(hasDynamicExceptionSpec);
   REGISTER_MATCHER(hasEitherOperand);
   REGISTER_MATCHER(hasElementType);
Index: clang/include/clang/ASTMatchers/ASTMatchers.h
===
--- clang/include/clang/ASTMatchers/ASTMatchers.h
+++ clang/include/clang/ASTMatchers/ASTMatchers.h
@@ -2862,7 +2862,7 @@
 /// BaseSpecMatcher.
 ///
 /// Example:
-/// matcher hasAnyBase(hasType(cxxRecordDecl(hasName("SpecialBase")
+/// matcher hasAnyBase(hasType(cxxRecordDecl(hasName("SpecialBase"
 /// \code
 ///   class Foo;
 ///   class Bar : Foo {};
@@ -2878,6 +2878,26 @@
   return internal::matchesAnyBase(Node, BaseSpecMatcher, Finder, Builder);
 }
 
+/// Matches C++ classes that have a direct base matching \p BaseSpecMatcher.
+///
+/// Example:
+/// matcher hasDirectBase(hasType(cxxRecordDecl(hasName("SpecialBase"
+/// \code
+///   class Foo;
+///   class Bar : Foo {};
+///   class Baz : Bar {};
+///   class SpecialBase;
+///   class Proxy : SpecialBase {};  // matches Proxy
+///   class IndirectlyDerived : Proxy {};  // doesn't match
+/// \endcode
+AST_MATCHER_P(CXXRecordDecl, hasDirectBase, internal::Matcher,
+  BaseSpecMatcher) {
+  return Node.hasDefinition() &&
+ llvm::any_of(Node.bases(), [&](const CXXBaseSpecifier &Base) {
+   return BaseSpecMatcher.matches(Base, Finder, Builder);
+ });
+}
+
 /// Similar to \c isDerivedFrom(), but also matches classes that directly
 /// match \c Base.
 AST_POLYMORPHIC_MATCHER_P_OVERLOAD(
Index: clang/docs/LibASTMatchersReference.html
===
--- clang/docs/LibASTMatchersReference.html
+++ clang/docs/LibASTMatchersReference.html
@@ -5658,7 +5658,7 @@
 Matches C++ classes that have a direct or indirect base matching BaseSpecMatcher.
 
 Example:
-matcher hasAnyBase(hasType(cxxRecordDecl(hasName("SpecialBase")
+matcher hasAnyBase(hasType(cxxRecordDecl(hasName("SpecialBase"
   class Foo;
   class Bar : Foo {};
   class Baz : Bar {};
@@ -5670,6 +5670,20 @@
 
 
 
+MatcherCXXRecordDecl>hasDirectBaseMatcherCXXBaseSpecifier> Ba

[PATCH] D81552: [ASTMatchers] Added hasDirectBase and hasClass Matchers

[Nathan James via Phabricator via cfe-commits]

njames93 updated this revision to Diff 275991.
njames93 added a comment.

Removed all hasClass related changes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81552

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

Index: clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
===
--- clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
+++ clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
@@ -3125,5 +3125,44 @@
  cxxRecordDecl(hasAnyBase(isVirtual();
 }
 
+TEST(BaseSpecifier, hasDirectBase) {
+  EXPECT_TRUE(matches(
+  R"cc(
+class Base {};
+class Derived : Base{};
+)cc",
+  cxxRecordDecl(hasName("Derived"),
+hasDirectBase(hasType(cxxRecordDecl(hasName("Base")));
+
+  StringRef MultiDerived = R"cc(
+class Base {};
+class Base2 {};
+class Derived : Base, Base2{};
+)cc";
+
+  EXPECT_TRUE(matches(
+  MultiDerived,
+  cxxRecordDecl(hasName("Derived"),
+hasDirectBase(hasType(cxxRecordDecl(hasName("Base")));
+  EXPECT_TRUE(matches(
+  MultiDerived,
+  cxxRecordDecl(hasName("Derived"),
+hasDirectBase(hasType(cxxRecordDecl(hasName("Base2")));
+
+  StringRef Indirect = R"cc(
+class Base {};
+class Intermediate : Base {};
+class Derived : Intermediate{};
+)cc";
+
+  EXPECT_TRUE(
+  matches(Indirect, cxxRecordDecl(hasName("Derived"),
+  hasDirectBase(hasType(cxxRecordDecl(
+  hasName("Intermediate")));
+  EXPECT_TRUE(notMatches(
+  Indirect,
+  cxxRecordDecl(hasName("Derived"),
+hasDirectBase(hasType(cxxRecordDecl(hasName("Base")));
+}
 } // namespace ast_matchers
 } // namespace clang
Index: clang/lib/ASTMatchers/Dynamic/Registry.cpp
===
--- clang/lib/ASTMatchers/Dynamic/Registry.cpp
+++ clang/lib/ASTMatchers/Dynamic/Registry.cpp
@@ -278,6 +278,7 @@
   REGISTER_MATCHER(hasDefinition);
   REGISTER_MATCHER(hasDescendant);
   REGISTER_MATCHER(hasDestinationType);
+  REGISTER_MATCHER(hasDirectBase);
   REGISTER_MATCHER(hasDynamicExceptionSpec);
   REGISTER_MATCHER(hasEitherOperand);
   REGISTER_MATCHER(hasElementType);
Index: clang/include/clang/ASTMatchers/ASTMatchers.h
===
--- clang/include/clang/ASTMatchers/ASTMatchers.h
+++ clang/include/clang/ASTMatchers/ASTMatchers.h
@@ -2862,7 +2862,7 @@
 /// BaseSpecMatcher.
 ///
 /// Example:
-/// matcher hasAnyBase(hasType(cxxRecordDecl(hasName("SpecialBase")
+/// matcher hasAnyBase(hasType(cxxRecordDecl(hasName("SpecialBase"
 /// \code
 ///   class Foo;
 ///   class Bar : Foo {};
@@ -2878,6 +2878,27 @@
   return internal::matchesAnyBase(Node, BaseSpecMatcher, Finder, Builder);
 }
 
+/// Matches C++ classes that have a direct base matching \p BaseSpecMatcher.
+///
+/// Example:
+/// matcher hasDirectBase(hasType(cxxRecordDecl(hasName("SpecialBase"
+/// \code
+///   class Foo;
+///   class Bar : Foo {};
+///   class Baz : Bar {};
+///   class SpecialBase;
+///   class Proxy : SpecialBase {};  // matches Proxy
+///   class IndirectlyDerived : Proxy {};  // doesn't match
+/// \endcode
+AST_MATCHER_P(CXXRecordDecl, hasDirectBase, internal::Matcher,
+  BaseSpecMatcher) {
+
+  return Node.hasDefinition() &&
+ llvm::any_of(Node.bases(), [&](const CXXBaseSpecifier &Base) {
+   return BaseSpecMatcher.matches(Base, Finder, Builder);
+ });
+}
+
 /// Similar to \c isDerivedFrom(), but also matches classes that directly
 /// match \c Base.
 AST_POLYMORPHIC_MATCHER_P_OVERLOAD(
Index: clang/docs/LibASTMatchersReference.html
===
--- clang/docs/LibASTMatchersReference.html
+++ clang/docs/LibASTMatchersReference.html
@@ -5658,7 +5658,7 @@
 Matches C++ classes that have a direct or indirect base matching BaseSpecMatcher.
 
 Example:
-matcher hasAnyBase(hasType(cxxRecordDecl(hasName("SpecialBase")
+matcher hasAnyBase(hasType(cxxRecordDecl(hasName("SpecialBase"
   class Foo;
   class Bar : Foo {};
   class Baz : Bar {};
@@ -5670,6 +5670,20 @@
 
 
 
+MatcherCXXRecordDecl>hasDirectBaseMatcherCXXBaseSpecifier> BaseSpecMatcher
+Matches C++ classes that have a direct base matching BaseSpecMatcher.
+
+Example:
+matcher hasDirectBase(hasType(cxxRecordDecl(hasName("SpecialBase"
+  class Foo;
+  class Bar : Foo {};
+  class Baz : Bar {}

[PATCH] D81552: [ASTMatchers] Added hasDirectBase and hasClass Matchers

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:3553
+/// \endcode
+AST_MATCHER_P(CXXBaseSpecifier, hasClass, internal::Matcher,
+  InnerMatcher) {

sammccall wrote:
> aaron.ballman wrote:
> > jkorous wrote:
> > > aaron.ballman wrote:
> > > > njames93 wrote:
> > > > > jkorous wrote:
> > > > > > aaron.ballman wrote:
> > > > > > > jkorous wrote:
> > > > > > > > Nit: while "[base specifier] `hasType`" sounds natural to me 
> > > > > > > > for some reason `hasClass` doesn't. English is not my first 
> > > > > > > > language though.
> > > > > > > I agree that `hasClass` seems unnatural here. Out of curiosity, 
> > > > > > > could we modify the `hasName` matcher to work on base specifiers 
> > > > > > > so you can write: `cxxRecordDecl(hasAnyBase(hasName("Base")))` as 
> > > > > > > shorthand for the more wordy version 
> > > > > > > `cxxRecordDecl(hasAnyBase(hasType(cxxRecordDecl(hasName("Base")`?
> > > > > > Wouldn't it be strange to treat `hasName` differently than all the 
> > > > > > other narrowing matchers? Honest question - I feel that `hasName` 
> > > > > > might be the most commonly used, just don't know if that's enough 
> > > > > > to justify this.
> > > > > > https://clang.llvm.org/docs/LibASTMatchersReference.html#narrowing-matchers
> > > > > Repurposing `hasName` would be a pain especially considering 99% of 
> > > > > its use cases wont be for base class matching.
> > > > > Wouldn't it be strange to treat hasName differently than all the 
> > > > > other narrowing matchers? Honest question - I feel that hasName might 
> > > > > be the most commonly used, just don't know if that's enough to 
> > > > > justify this. 
> > > > > https://clang.llvm.org/docs/LibASTMatchersReference.html#narrowing-matchers
> > > > 
> > > > Different how? I'm suggesting to overload `hasName` to work on a 
> > > > `CXXBaseSpecifier` since those have a name.
> > > > 
> > > > > Repurposing hasName would be a pain especially considering 99% of its 
> > > > > use cases wont be for base class matching.
> > > > 
> > > > I'm asking what the right API is for users, though, which is a bit 
> > > > different. Base specifiers have names (there are no unnamed base 
> > > > specifiers), so to me, it makes more sense for `hasName` to work with 
> > > > them directly since that is the thing that does name matching.
> > > > 
> > > > I think you can accomplish this by using a 
> > > > `PolymorphicMatcherWithParam1` like we do for 
> > > > `hasOverloadedOperatorName` which can narrow to either a 
> > > > `CXXOperatorCallExpr` or a `FunctionDecl`.
> > > >> Wouldn't it be strange to treat hasName differently than all the other 
> > > >> narrowing matchers? Honest question - I feel that hasName might be the 
> > > >> most commonly used, just don't know if that's enough to justify this. 
> > > >> https://clang.llvm.org/docs/LibASTMatchersReference.html#narrowing-matchers
> > > 
> > > > Different how? I'm suggesting to overload hasName to work on a 
> > > > CXXBaseSpecifier since those have a name.
> > > 
> > > What I meant is that technically we can overload any 
> > > `Matcher` matcher in the same fashion so having the 
> > > overloaded version of `hasName` only makes it somewhat special (and 
> > > someone might argue that it'd impact consistency of matchers 
> > > composability). Anyway, I'm fine with your suggestion!
> > Ah, I see what you mean -- thanks for explaining. I'm on the fence about 
> > this. One the one hand, base specifiers *in the AST* do not have names, so 
> > it seems defensible to say that `hasName` should not handle a base 
> > specifier. On the other hand, base specifiers *in the language* are 
> > identifiers that always have a name, so it seems defensible to say that 
> > `hashName` should handle a base specifier.
> > 
> > Pulling in some more folks to see if a wider audience brings consensus.
> I think I agree that it's reasonable/consistent to do this, but I'm not sure 
> it's a good idea.
> Base specifier -> type -> class -> name seems like the most *regular* 
> traversal that reflects the shape/concepts of the AST, shortcutting steps 
> makes the model (IMO) more complicated to make it more terse.
> Is matching a base specifier with a hardcoded name really common enough in 
> absolute terms that people should keep this special case in their heads?
> 
> I think `hasType(cxxRecordDecl(hasName("Base")))` a bit better, but i think 
> of *expressions* having types, and would prefer `specifiesType` or just 
> `specifies` at the outside.
> But just my 2c and I'm not deeply familiar with the conventions here - I can 
> live with any of the options.
> Is matching a base specifier with a hardcoded name really common enough in 
> absolute terms that people should keep this special case in their heads?

Thanks for the perspective. I don't think base specifiers will be matched all 
that often, but I'm also not certain it's a speci

[PATCH] D81552: [ASTMatchers] Added hasDirectBase and hasClass Matchers

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.



Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:3553
+/// \endcode
+AST_MATCHER_P(CXXBaseSpecifier, hasClass, internal::Matcher,
+  InnerMatcher) {

aaron.ballman wrote:
> jkorous wrote:
> > aaron.ballman wrote:
> > > njames93 wrote:
> > > > jkorous wrote:
> > > > > aaron.ballman wrote:
> > > > > > jkorous wrote:
> > > > > > > Nit: while "[base specifier] `hasType`" sounds natural to me for 
> > > > > > > some reason `hasClass` doesn't. English is not my first language 
> > > > > > > though.
> > > > > > I agree that `hasClass` seems unnatural here. Out of curiosity, 
> > > > > > could we modify the `hasName` matcher to work on base specifiers so 
> > > > > > you can write: `cxxRecordDecl(hasAnyBase(hasName("Base")))` as 
> > > > > > shorthand for the more wordy version 
> > > > > > `cxxRecordDecl(hasAnyBase(hasType(cxxRecordDecl(hasName("Base")`?
> > > > > Wouldn't it be strange to treat `hasName` differently than all the 
> > > > > other narrowing matchers? Honest question - I feel that `hasName` 
> > > > > might be the most commonly used, just don't know if that's enough to 
> > > > > justify this.
> > > > > https://clang.llvm.org/docs/LibASTMatchersReference.html#narrowing-matchers
> > > > Repurposing `hasName` would be a pain especially considering 99% of its 
> > > > use cases wont be for base class matching.
> > > > Wouldn't it be strange to treat hasName differently than all the other 
> > > > narrowing matchers? Honest question - I feel that hasName might be the 
> > > > most commonly used, just don't know if that's enough to justify this. 
> > > > https://clang.llvm.org/docs/LibASTMatchersReference.html#narrowing-matchers
> > > 
> > > Different how? I'm suggesting to overload `hasName` to work on a 
> > > `CXXBaseSpecifier` since those have a name.
> > > 
> > > > Repurposing hasName would be a pain especially considering 99% of its 
> > > > use cases wont be for base class matching.
> > > 
> > > I'm asking what the right API is for users, though, which is a bit 
> > > different. Base specifiers have names (there are no unnamed base 
> > > specifiers), so to me, it makes more sense for `hasName` to work with 
> > > them directly since that is the thing that does name matching.
> > > 
> > > I think you can accomplish this by using a `PolymorphicMatcherWithParam1` 
> > > like we do for `hasOverloadedOperatorName` which can narrow to either a 
> > > `CXXOperatorCallExpr` or a `FunctionDecl`.
> > >> Wouldn't it be strange to treat hasName differently than all the other 
> > >> narrowing matchers? Honest question - I feel that hasName might be the 
> > >> most commonly used, just don't know if that's enough to justify this. 
> > >> https://clang.llvm.org/docs/LibASTMatchersReference.html#narrowing-matchers
> > 
> > > Different how? I'm suggesting to overload hasName to work on a 
> > > CXXBaseSpecifier since those have a name.
> > 
> > What I meant is that technically we can overload any 
> > `Matcher` matcher in the same fashion so having the 
> > overloaded version of `hasName` only makes it somewhat special (and someone 
> > might argue that it'd impact consistency of matchers composability). 
> > Anyway, I'm fine with your suggestion!
> Ah, I see what you mean -- thanks for explaining. I'm on the fence about 
> this. One the one hand, base specifiers *in the AST* do not have names, so it 
> seems defensible to say that `hasName` should not handle a base specifier. On 
> the other hand, base specifiers *in the language* are identifiers that always 
> have a name, so it seems defensible to say that `hashName` should handle a 
> base specifier.
> 
> Pulling in some more folks to see if a wider audience brings consensus.
I think I agree that it's reasonable/consistent to do this, but I'm not sure 
it's a good idea.
Base specifier -> type -> class -> name seems like the most *regular* traversal 
that reflects the shape/concepts of the AST, shortcutting steps makes the model 
(IMO) more complicated to make it more terse.
Is matching a base specifier with a hardcoded name really common enough in 
absolute terms that people should keep this special case in their heads?

I think `hasType(cxxRecordDecl(hasName("Base")))` a bit better, but i think of 
*expressions* having types, and would prefer `specifiesType` or just 
`specifies` at the outside.
But just my 2c and I'm not deeply familiar with the conventions here - I can 
live with any of the options.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81552



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

[PATCH] D81552: [ASTMatchers] Added hasDirectBase and hasClass Matchers

[Manuel Klimek via Phabricator via cfe-commits]

klimek added a comment.

We have some precedent for overloading has* matchers, but I'll defer to Sam's 
judgement whether that's a good idea here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81552



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

[PATCH] D81552: [ASTMatchers] Added hasDirectBase and hasClass Matchers

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added reviewers: sammccall, dblaikie.
aaron.ballman added subscribers: sammccall, dblaikie.
aaron.ballman added a comment.

Pinging @klimek , @sammccall , and @dblaikie to see if there are some opinions 
about overloading `hasName` (and possibly other naming related questions).




Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:3553
+/// \endcode
+AST_MATCHER_P(CXXBaseSpecifier, hasClass, internal::Matcher,
+  InnerMatcher) {

jkorous wrote:
> aaron.ballman wrote:
> > njames93 wrote:
> > > jkorous wrote:
> > > > aaron.ballman wrote:
> > > > > jkorous wrote:
> > > > > > Nit: while "[base specifier] `hasType`" sounds natural to me for 
> > > > > > some reason `hasClass` doesn't. English is not my first language 
> > > > > > though.
> > > > > I agree that `hasClass` seems unnatural here. Out of curiosity, could 
> > > > > we modify the `hasName` matcher to work on base specifiers so you can 
> > > > > write: `cxxRecordDecl(hasAnyBase(hasName("Base")))` as shorthand for 
> > > > > the more wordy version 
> > > > > `cxxRecordDecl(hasAnyBase(hasType(cxxRecordDecl(hasName("Base")`?
> > > > Wouldn't it be strange to treat `hasName` differently than all the 
> > > > other narrowing matchers? Honest question - I feel that `hasName` might 
> > > > be the most commonly used, just don't know if that's enough to justify 
> > > > this.
> > > > https://clang.llvm.org/docs/LibASTMatchersReference.html#narrowing-matchers
> > > Repurposing `hasName` would be a pain especially considering 99% of its 
> > > use cases wont be for base class matching.
> > > Wouldn't it be strange to treat hasName differently than all the other 
> > > narrowing matchers? Honest question - I feel that hasName might be the 
> > > most commonly used, just don't know if that's enough to justify this. 
> > > https://clang.llvm.org/docs/LibASTMatchersReference.html#narrowing-matchers
> > 
> > Different how? I'm suggesting to overload `hasName` to work on a 
> > `CXXBaseSpecifier` since those have a name.
> > 
> > > Repurposing hasName would be a pain especially considering 99% of its use 
> > > cases wont be for base class matching.
> > 
> > I'm asking what the right API is for users, though, which is a bit 
> > different. Base specifiers have names (there are no unnamed base 
> > specifiers), so to me, it makes more sense for `hasName` to work with them 
> > directly since that is the thing that does name matching.
> > 
> > I think you can accomplish this by using a `PolymorphicMatcherWithParam1` 
> > like we do for `hasOverloadedOperatorName` which can narrow to either a 
> > `CXXOperatorCallExpr` or a `FunctionDecl`.
> >> Wouldn't it be strange to treat hasName differently than all the other 
> >> narrowing matchers? Honest question - I feel that hasName might be the 
> >> most commonly used, just don't know if that's enough to justify this. 
> >> https://clang.llvm.org/docs/LibASTMatchersReference.html#narrowing-matchers
> 
> > Different how? I'm suggesting to overload hasName to work on a 
> > CXXBaseSpecifier since those have a name.
> 
> What I meant is that technically we can overload any `Matcher` 
> matcher in the same fashion so having the overloaded version of `hasName` 
> only makes it somewhat special (and someone might argue that it'd impact 
> consistency of matchers composability). Anyway, I'm fine with your suggestion!
Ah, I see what you mean -- thanks for explaining. I'm on the fence about this. 
One the one hand, base specifiers *in the AST* do not have names, so it seems 
defensible to say that `hasName` should not handle a base specifier. On the 
other hand, base specifiers *in the language* are identifiers that always have 
a name, so it seems defensible to say that `hashName` should handle a base 
specifier.

Pulling in some more folks to see if a wider audience brings consensus.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81552



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

[PATCH] D81552: [ASTMatchers] Added hasDirectBase and hasClass Matchers

[Jan Korous via Phabricator via cfe-commits]

jkorous added inline comments.



Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:3553
+/// \endcode
+AST_MATCHER_P(CXXBaseSpecifier, hasClass, internal::Matcher,
+  InnerMatcher) {

aaron.ballman wrote:
> njames93 wrote:
> > jkorous wrote:
> > > aaron.ballman wrote:
> > > > jkorous wrote:
> > > > > Nit: while "[base specifier] `hasType`" sounds natural to me for some 
> > > > > reason `hasClass` doesn't. English is not my first language though.
> > > > I agree that `hasClass` seems unnatural here. Out of curiosity, could 
> > > > we modify the `hasName` matcher to work on base specifiers so you can 
> > > > write: `cxxRecordDecl(hasAnyBase(hasName("Base")))` as shorthand for 
> > > > the more wordy version 
> > > > `cxxRecordDecl(hasAnyBase(hasType(cxxRecordDecl(hasName("Base")`?
> > > Wouldn't it be strange to treat `hasName` differently than all the other 
> > > narrowing matchers? Honest question - I feel that `hasName` might be the 
> > > most commonly used, just don't know if that's enough to justify this.
> > > https://clang.llvm.org/docs/LibASTMatchersReference.html#narrowing-matchers
> > Repurposing `hasName` would be a pain especially considering 99% of its use 
> > cases wont be for base class matching.
> > Wouldn't it be strange to treat hasName differently than all the other 
> > narrowing matchers? Honest question - I feel that hasName might be the most 
> > commonly used, just don't know if that's enough to justify this. 
> > https://clang.llvm.org/docs/LibASTMatchersReference.html#narrowing-matchers
> 
> Different how? I'm suggesting to overload `hasName` to work on a 
> `CXXBaseSpecifier` since those have a name.
> 
> > Repurposing hasName would be a pain especially considering 99% of its use 
> > cases wont be for base class matching.
> 
> I'm asking what the right API is for users, though, which is a bit different. 
> Base specifiers have names (there are no unnamed base specifiers), so to me, 
> it makes more sense for `hasName` to work with them directly since that is 
> the thing that does name matching.
> 
> I think you can accomplish this by using a `PolymorphicMatcherWithParam1` 
> like we do for `hasOverloadedOperatorName` which can narrow to either a 
> `CXXOperatorCallExpr` or a `FunctionDecl`.
>> Wouldn't it be strange to treat hasName differently than all the other 
>> narrowing matchers? Honest question - I feel that hasName might be the most 
>> commonly used, just don't know if that's enough to justify this. 
>> https://clang.llvm.org/docs/LibASTMatchersReference.html#narrowing-matchers

> Different how? I'm suggesting to overload hasName to work on a 
> CXXBaseSpecifier since those have a name.

What I meant is that technically we can overload any `Matcher` 
matcher in the same fashion so having the overloaded version of `hasName` only 
makes it somewhat special (and someone might argue that it'd impact consistency 
of matchers composability). Anyway, I'm fine with your suggestion!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81552



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

[PATCH] D81552: [ASTMatchers] Added hasDirectBase and hasClass Matchers

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:3553
+/// \endcode
+AST_MATCHER_P(CXXBaseSpecifier, hasClass, internal::Matcher,
+  InnerMatcher) {

njames93 wrote:
> jkorous wrote:
> > aaron.ballman wrote:
> > > jkorous wrote:
> > > > Nit: while "[base specifier] `hasType`" sounds natural to me for some 
> > > > reason `hasClass` doesn't. English is not my first language though.
> > > I agree that `hasClass` seems unnatural here. Out of curiosity, could we 
> > > modify the `hasName` matcher to work on base specifiers so you can write: 
> > > `cxxRecordDecl(hasAnyBase(hasName("Base")))` as shorthand for the more 
> > > wordy version 
> > > `cxxRecordDecl(hasAnyBase(hasType(cxxRecordDecl(hasName("Base")`?
> > Wouldn't it be strange to treat `hasName` differently than all the other 
> > narrowing matchers? Honest question - I feel that `hasName` might be the 
> > most commonly used, just don't know if that's enough to justify this.
> > https://clang.llvm.org/docs/LibASTMatchersReference.html#narrowing-matchers
> Repurposing `hasName` would be a pain especially considering 99% of its use 
> cases wont be for base class matching.
> Wouldn't it be strange to treat hasName differently than all the other 
> narrowing matchers? Honest question - I feel that hasName might be the most 
> commonly used, just don't know if that's enough to justify this. 
> https://clang.llvm.org/docs/LibASTMatchersReference.html#narrowing-matchers

Different how? I'm suggesting to overload `hasName` to work on a 
`CXXBaseSpecifier` since those have a name.

> Repurposing hasName would be a pain especially considering 99% of its use 
> cases wont be for base class matching.

I'm asking what the right API is for users, though, which is a bit different. 
Base specifiers have names (there are no unnamed base specifiers), so to me, it 
makes more sense for `hasName` to work with them directly since that is the 
thing that does name matching.

I think you can accomplish this by using a `PolymorphicMatcherWithParam1` like 
we do for `hasOverloadedOperatorName` which can narrow to either a 
`CXXOperatorCallExpr` or a `FunctionDecl`.



Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:3621
+/// \endcode
+AST_MATCHER_P(CXXBaseSpecifier, hasClassOrClassTemplate,
+  internal::Matcher, InnerMatcher) {

jkorous wrote:
> I think we should just use `eachOf` matcher for this kind of composition.
> 
> https://clang.llvm.org/docs/LibASTMatchersReference.html#traversal-matchers
+1


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81552



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

[PATCH] D81552: [ASTMatchers] Added hasDirectBase and hasClass Matchers

[Nathan James via Phabricator via cfe-commits]

njames93 marked 2 inline comments as done.
njames93 added inline comments.



Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:3537
 AST_POLYMORPHIC_MATCHER_P_OVERLOAD(
-hasType,
-AST_POLYMORPHIC_SUPPORTED_TYPES(Expr, FriendDecl, ValueDecl,
-CXXBaseSpecifier),
+hasType, AST_POLYMORPHIC_SUPPORTED_TYPES(Expr, FriendDecl, ValueDecl),
 internal::Matcher, InnerMatcher, 1) {

aaron.ballman wrote:
> njames93 wrote:
> > jkorous wrote:
> > > aaron.ballman wrote:
> > > > njames93 wrote:
> > > > > aaron.ballman wrote:
> > > > > > This is undoing a change that was just added less than two weeks 
> > > > > > ago, so I think the potential for breaking code is small. That 
> > > > > > said, can you explain why you think `hasClass` is a better approach 
> > > > > > than `hasType`?
> > > > > Yeah, as that change hasn't reached landed onto a release branch 
> > > > > breaking code shouldn't be an issue, If it was I'd leave it in.
> > > > > 
> > > > > - `hasType` is very generic, whereas `hasClass` is specific to what a 
> > > > > `CXXBaseSpecifier` supports.
> > > > > - It makes the matchers marginally simpler.
> > > > >   `hasDirectBase(hasType(cxxRecordDecl(hasName("Base"` vs 
> > > > > `hasDirectBase(hasClass(hasName("Base")))`
> > > > > - In the documentation it also specifies that `hasClass` takes a 
> > > > > `Matcher, making it more user friendly.
> > > > FWIW, I prefer `hasType` to `hasClass`. You can inherit from things 
> > > > which are not a class, such as a struct (so the name is a bit of a 
> > > > misnomer, but not too awful), a class template (which you can't match 
> > > > with this interface), or a template type (which you also can't match 
> > > > with this interface).
> > > I don't feel super strongly about this but I also slightly prefer 
> > > `hasType`.
> > > 
> > > To be fair - I didn't really have things like inheritance from template 
> > > parameters on my mind when working on `hasAnyBase` (it's definitely not 
> > > tested) so I'd rather not assume it works.
> > I have decided to put `hasType` back in there as it does have some general 
> > uses. However I have added more class and class template specific matchers 
> > as I feel these are slightly more user friendly. 
> > 
> > LMK what you think of this approach.
> > 
> > Side note what is the correct collective term for classes and structs. I'd 
> > be tempted to refer to them how clang does, records, but `hasRecord` seems 
> > wrong.
> > Side note what is the correct collective term for classes and structs. I'd 
> > be tempted to refer to them how clang does, records, but hasRecord seems 
> > wrong.
> 
> We use the term "record", but I'm not certain how widely used that is.
https://en.cppreference.com/w/cpp/language/class - Going of what that says, it 
states that a class declaration starts with a keyword either `class` or 
`struct`. Nowhere on the page does it mention `record`.
Continuing on from this point, we have many more matchers with `class` in the 
name but work on structs too:
`ofClass`, `hasInClassInitializer` and `injectedClassNameType`. If you're being 
pedantic there is also `classTemplateDecl`, 
`classTemplatePartialSpecializationDecl` and `classTemplateSpecializationDecl`.

Having said all of that I'm still not a huge fan of `hasClass`, but I'm less of 
a fan of `hasType`. I'd thought of `forClass` but that could be misinterpreted 
as the derived class of the `CXXBaseSpecifier` Kind of like the behaviour of 
`forFunction`.
```
class Base {};
class Derived : Base {};
```
Does `cxxBaseSpecifier(forClass(cxxRecordDecl().bind("X")) bind to `Derived` or 
`Base`?



Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:3553
+/// \endcode
+AST_MATCHER_P(CXXBaseSpecifier, hasClass, internal::Matcher,
+  InnerMatcher) {

jkorous wrote:
> aaron.ballman wrote:
> > jkorous wrote:
> > > Nit: while "[base specifier] `hasType`" sounds natural to me for some 
> > > reason `hasClass` doesn't. English is not my first language though.
> > I agree that `hasClass` seems unnatural here. Out of curiosity, could we 
> > modify the `hasName` matcher to work on base specifiers so you can write: 
> > `cxxRecordDecl(hasAnyBase(hasName("Base")))` as shorthand for the more 
> > wordy version 
> > `cxxRecordDecl(hasAnyBase(hasType(cxxRecordDecl(hasName("Base")`?
> Wouldn't it be strange to treat `hasName` differently than all the other 
> narrowing matchers? Honest question - I feel that `hasName` might be the most 
> commonly used, just don't know if that's enough to justify this.
> https://clang.llvm.org/docs/LibASTMatchersReference.html#narrowing-matchers
Repurposing `hasName` would be a pain especially considering 99% of its use 
cases wont be for base class matching.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81552



__

[PATCH] D81552: [ASTMatchers] Added hasDirectBase and hasClass Matchers

[Jan Korous via Phabricator via cfe-commits]

jkorous added inline comments.



Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:3553
+/// \endcode
+AST_MATCHER_P(CXXBaseSpecifier, hasClass, internal::Matcher,
+  InnerMatcher) {

aaron.ballman wrote:
> jkorous wrote:
> > Nit: while "[base specifier] `hasType`" sounds natural to me for some 
> > reason `hasClass` doesn't. English is not my first language though.
> I agree that `hasClass` seems unnatural here. Out of curiosity, could we 
> modify the `hasName` matcher to work on base specifiers so you can write: 
> `cxxRecordDecl(hasAnyBase(hasName("Base")))` as shorthand for the more wordy 
> version `cxxRecordDecl(hasAnyBase(hasType(cxxRecordDecl(hasName("Base")`?
Wouldn't it be strange to treat `hasName` differently than all the other 
narrowing matchers? Honest question - I feel that `hasName` might be the most 
commonly used, just don't know if that's enough to justify this.
https://clang.llvm.org/docs/LibASTMatchersReference.html#narrowing-matchers


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81552



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

[PATCH] D81552: [ASTMatchers] Added hasDirectBase and hasClass Matchers

[Jan Korous via Phabricator via cfe-commits]

jkorous added inline comments.



Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:3621
+/// \endcode
+AST_MATCHER_P(CXXBaseSpecifier, hasClassOrClassTemplate,
+  internal::Matcher, InnerMatcher) {

I think we should just use `eachOf` matcher for this kind of composition.

https://clang.llvm.org/docs/LibASTMatchersReference.html#traversal-matchers


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81552



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

[PATCH] D81552: [ASTMatchers] Added hasDirectBase and hasClass Matchers

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:3537
 AST_POLYMORPHIC_MATCHER_P_OVERLOAD(
-hasType,
-AST_POLYMORPHIC_SUPPORTED_TYPES(Expr, FriendDecl, ValueDecl,
-CXXBaseSpecifier),
+hasType, AST_POLYMORPHIC_SUPPORTED_TYPES(Expr, FriendDecl, ValueDecl),
 internal::Matcher, InnerMatcher, 1) {

njames93 wrote:
> jkorous wrote:
> > aaron.ballman wrote:
> > > njames93 wrote:
> > > > aaron.ballman wrote:
> > > > > This is undoing a change that was just added less than two weeks ago, 
> > > > > so I think the potential for breaking code is small. That said, can 
> > > > > you explain why you think `hasClass` is a better approach than 
> > > > > `hasType`?
> > > > Yeah, as that change hasn't reached landed onto a release branch 
> > > > breaking code shouldn't be an issue, If it was I'd leave it in.
> > > > 
> > > > - `hasType` is very generic, whereas `hasClass` is specific to what a 
> > > > `CXXBaseSpecifier` supports.
> > > > - It makes the matchers marginally simpler.
> > > >   `hasDirectBase(hasType(cxxRecordDecl(hasName("Base"` vs 
> > > > `hasDirectBase(hasClass(hasName("Base")))`
> > > > - In the documentation it also specifies that `hasClass` takes a 
> > > > `Matcher, making it more user friendly.
> > > FWIW, I prefer `hasType` to `hasClass`. You can inherit from things which 
> > > are not a class, such as a struct (so the name is a bit of a misnomer, 
> > > but not too awful), a class template (which you can't match with this 
> > > interface), or a template type (which you also can't match with this 
> > > interface).
> > I don't feel super strongly about this but I also slightly prefer `hasType`.
> > 
> > To be fair - I didn't really have things like inheritance from template 
> > parameters on my mind when working on `hasAnyBase` (it's definitely not 
> > tested) so I'd rather not assume it works.
> I have decided to put `hasType` back in there as it does have some general 
> uses. However I have added more class and class template specific matchers as 
> I feel these are slightly more user friendly. 
> 
> LMK what you think of this approach.
> 
> Side note what is the correct collective term for classes and structs. I'd be 
> tempted to refer to them how clang does, records, but `hasRecord` seems wrong.
> Side note what is the correct collective term for classes and structs. I'd be 
> tempted to refer to them how clang does, records, but hasRecord seems wrong.

We use the term "record", but I'm not certain how widely used that is.



Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:3553
+/// \endcode
+AST_MATCHER_P(CXXBaseSpecifier, hasClass, internal::Matcher,
+  InnerMatcher) {

jkorous wrote:
> Nit: while "[base specifier] `hasType`" sounds natural to me for some reason 
> `hasClass` doesn't. English is not my first language though.
I agree that `hasClass` seems unnatural here. Out of curiosity, could we modify 
the `hasName` matcher to work on base specifiers so you can write: 
`cxxRecordDecl(hasAnyBase(hasName("Base")))` as shorthand for the more wordy 
version `cxxRecordDecl(hasAnyBase(hasType(cxxRecordDecl(hasName("Base")`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81552



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

[PATCH] D81552: [ASTMatchers] Added hasDirectBase and hasClass Matchers

[Nathan James via Phabricator via cfe-commits]

njames93 marked 2 inline comments as done.
njames93 added a comment.

In D81552#2086420 , @jkorous wrote:

> @njames93 `hasDirectBase` seems like a useful matcher to me! OTOH I am not 
> totally convinced about `hasType` -> `hasClass`. Anyway, don't you want to 
> land `hasDirectBase` as a separate patch first and then discuss the rest?


There more I add to this, the more splitting it up sounds like a good idea.

> One more thing - I'm just thinking if there isn't some corner case where a 
> base class could be interpreted as both direct and indirect. The most ugly 
> case I came up with is virtual inheritance. I admit I don't know what the 
> standard says about this so it might be a non-issue. Also, it'd still 
> probably make sense to match on such base. WDYT?
> 
>   struct Base {};
>   struct Proxy : virtual Base {};
>   struct Derived : Base, Proxy {};

In that case it would probably register as a direct and indirect base. However 
there is no matcher(nor much need for one) that will solely match indirect 
bases so its mostly a non issue.




Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:3537
 AST_POLYMORPHIC_MATCHER_P_OVERLOAD(
-hasType,
-AST_POLYMORPHIC_SUPPORTED_TYPES(Expr, FriendDecl, ValueDecl,
-CXXBaseSpecifier),
+hasType, AST_POLYMORPHIC_SUPPORTED_TYPES(Expr, FriendDecl, ValueDecl),
 internal::Matcher, InnerMatcher, 1) {

jkorous wrote:
> aaron.ballman wrote:
> > njames93 wrote:
> > > aaron.ballman wrote:
> > > > This is undoing a change that was just added less than two weeks ago, 
> > > > so I think the potential for breaking code is small. That said, can you 
> > > > explain why you think `hasClass` is a better approach than `hasType`?
> > > Yeah, as that change hasn't reached landed onto a release branch breaking 
> > > code shouldn't be an issue, If it was I'd leave it in.
> > > 
> > > - `hasType` is very generic, whereas `hasClass` is specific to what a 
> > > `CXXBaseSpecifier` supports.
> > > - It makes the matchers marginally simpler.
> > >   `hasDirectBase(hasType(cxxRecordDecl(hasName("Base"` vs 
> > > `hasDirectBase(hasClass(hasName("Base")))`
> > > - In the documentation it also specifies that `hasClass` takes a 
> > > `Matcher, making it more user friendly.
> > FWIW, I prefer `hasType` to `hasClass`. You can inherit from things which 
> > are not a class, such as a struct (so the name is a bit of a misnomer, but 
> > not too awful), a class template (which you can't match with this 
> > interface), or a template type (which you also can't match with this 
> > interface).
> I don't feel super strongly about this but I also slightly prefer `hasType`.
> 
> To be fair - I didn't really have things like inheritance from template 
> parameters on my mind when working on `hasAnyBase` (it's definitely not 
> tested) so I'd rather not assume it works.
I have decided to put `hasType` back in there as it does have some general 
uses. However I have added more class and class template specific matchers as I 
feel these are slightly more user friendly. 

LMK what you think of this approach.

Side note what is the correct collective term for classes and structs. I'd be 
tempted to refer to them how clang does, records, but `hasRecord` seems wrong.



Comment at: clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp:3158
+class Base {};
+class Derived : BAse {};
+  )cc",

jkorous wrote:
> Is this (`Base` != `BAse`) a typo or a way how to tease the asserts?
It was to tease the assers, but I removed it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81552



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

[PATCH] D81552: [ASTMatchers] Added hasDirectBase and hasClass Matchers

[Nathan James via Phabricator via cfe-commits]

njames93 updated this revision to Diff 27.
njames93 added a comment.

- Added back `hasType` overload for `CXXBaseSpecifier`
- Added `hasClassTemplate` and `hasClassOrClassTemplate` matcher for 
`CXXBaseSpecifier`
- Added `hasTemplatedDecl` for `ClassTemplateDecl`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81552

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

Index: clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
===
--- clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
+++ clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
@@ -2997,26 +2997,24 @@
 }
 
 TEST(HasAnyBase, DirectBase) {
-  EXPECT_TRUE(matches(
-  "struct Base {};"
-  "struct ExpectedMatch : Base {};",
-  cxxRecordDecl(hasName("ExpectedMatch"),
-hasAnyBase(hasType(cxxRecordDecl(hasName("Base")));
+  EXPECT_TRUE(matches("struct Base {};"
+  "struct ExpectedMatch : Base {};",
+  cxxRecordDecl(hasName("ExpectedMatch"),
+hasAnyBase(hasClass(hasName("Base"));
 }
 
 TEST(HasAnyBase, IndirectBase) {
-  EXPECT_TRUE(matches(
-  "struct Base {};"
-  "struct Intermediate : Base {};"
-  "struct ExpectedMatch : Intermediate {};",
-  cxxRecordDecl(hasName("ExpectedMatch"),
-hasAnyBase(hasType(cxxRecordDecl(hasName("Base")));
+  EXPECT_TRUE(matches("struct Base {};"
+  "struct Intermediate : Base {};"
+  "struct ExpectedMatch : Intermediate {};",
+  cxxRecordDecl(hasName("ExpectedMatch"),
+hasAnyBase(hasClass(hasName("Base"));
 }
 
 TEST(HasAnyBase, NoBase) {
   EXPECT_TRUE(notMatches("struct Foo {};"
  "struct Bar {};",
- cxxRecordDecl(hasAnyBase(hasType(cxxRecordDecl());
+ cxxRecordDecl(hasAnyBase(hasClass(cxxRecordDecl());
 }
 
 TEST(IsPublicBase, Public) {
@@ -3117,5 +3115,94 @@
  cxxRecordDecl(hasAnyBase(isVirtual();
 }
 
+TEST(BaseSpecifier, hasDirectBase) {
+  EXPECT_TRUE(matches(
+  R"cc(
+class Base {};
+class Derived : Base{};
+)cc",
+  cxxRecordDecl(hasName("Derived"),
+hasDirectBase(hasClass(hasName("Base"));
+
+  StringRef MultiDerived = R"cc(
+class Base {};
+class Base2 {};
+class Derived : Base, Base2{};
+)cc";
+
+  EXPECT_TRUE(matches(MultiDerived,
+  cxxRecordDecl(hasName("Derived"),
+hasDirectBase(hasClass(hasName("Base"));
+  EXPECT_TRUE(matches(
+  MultiDerived, cxxRecordDecl(hasName("Derived"),
+  hasDirectBase(hasClass(hasName("Base2"));
+
+  StringRef Indirect = R"cc(
+class Base {};
+class Intermediate : Base {};
+class Derived : Intermediate{};
+)cc";
+
+  EXPECT_TRUE(
+  matches(Indirect,
+  cxxRecordDecl(hasName("Derived"),
+hasDirectBase(hasClass(hasName("Intermediate"));
+  EXPECT_TRUE(notMatches(
+  Indirect, cxxRecordDecl(hasName("Derived"),
+  hasDirectBase(hasClass(hasName("Base"));
+}
+
+TEST(BaseSpecifier, HasClassTemplate) {
+  DeclarationMatcher Matcher = cxxRecordDecl(
+  hasDirectBase(hasClassTemplate(hasTemplatedDecl(hasName("Base");
+  EXPECT_TRUE(matches(R"cc(
+template class Base {};
+template class Derived : Base {};
+  )cc",
+  Matcher));
+  EXPECT_FALSE(matches(R"cc(
+template class Base {};
+template class Derived : Base {};
+  )cc",
+   Matcher));
+}
+
+TEST(BaseSpecifier, HasClassOrClassTemplate) {
+  DeclarationMatcher Matcher =
+  cxxRecordDecl(hasDirectBase(hasClassOrClassTemplate(hasName("Base";
+  EXPECT_TRUE(matches(R"cc(
+template class Base {};
+template class Derived : Base {};
+  )cc",
+  Matcher));
+  EXPECT_TRUE(matches(R"cc(
+template class Base {};
+template class Derived : Base {};
+  )cc",
+  Matcher));
+  EXPECT_TRUE(matches(R"cc(
+template class Base {};
+class Derived : Base {};
+  )cc",
+  Matcher));
+  EXPECT_TRUE(matches(R"cc(
+class Base {};
+template class Derived : Base {};
+  )cc",
+  Matcher));
+  EXPECT_TRUE(matches(R"cc(
+class Base {};
+class Derived : Base {};
+  )cc",
+  Matcher));
+}
+
+TEST(ClassTemplateDecl, HasTemplatedDecl) {
+  DeclarationMatcher Matcher =
+  classTemplateDecl(hasTemplatedDecl(hasName("A")));
+  EXPECT_

[PATCH] D81552: [ASTMatchers] Added hasDirectBase and hasClass Matchers

[Jan Korous via Phabricator via cfe-commits]

jkorous added a comment.

@njames93 `hasDirectBase` seems like a useful matcher to me! OTOH I am not 
totally convinced about `hasType` -> `hasClass`. Anyway, don't you want to land 
`hasDirectBase` as a separate patch first and then discuss the rest?

One more thing - I'm just thinking if there isn't some corner case where a base 
class could be interpreted as both direct and indirect. The most ugly case I 
came up with is virtual inheritance. I admit I don't know what the standard 
says about this so it might be a non-issue. Also, it'd still probably make 
sense to match on such base. WDYT?

  struct Base {};
  struct Proxy : virtual Base {};
  struct Derived : Base, Proxy {};




Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:3537
 AST_POLYMORPHIC_MATCHER_P_OVERLOAD(
-hasType,
-AST_POLYMORPHIC_SUPPORTED_TYPES(Expr, FriendDecl, ValueDecl,
-CXXBaseSpecifier),
+hasType, AST_POLYMORPHIC_SUPPORTED_TYPES(Expr, FriendDecl, ValueDecl),
 internal::Matcher, InnerMatcher, 1) {

aaron.ballman wrote:
> njames93 wrote:
> > aaron.ballman wrote:
> > > This is undoing a change that was just added less than two weeks ago, so 
> > > I think the potential for breaking code is small. That said, can you 
> > > explain why you think `hasClass` is a better approach than `hasType`?
> > Yeah, as that change hasn't reached landed onto a release branch breaking 
> > code shouldn't be an issue, If it was I'd leave it in.
> > 
> > - `hasType` is very generic, whereas `hasClass` is specific to what a 
> > `CXXBaseSpecifier` supports.
> > - It makes the matchers marginally simpler.
> >   `hasDirectBase(hasType(cxxRecordDecl(hasName("Base"` vs 
> > `hasDirectBase(hasClass(hasName("Base")))`
> > - In the documentation it also specifies that `hasClass` takes a 
> > `Matcher, making it more user friendly.
> FWIW, I prefer `hasType` to `hasClass`. You can inherit from things which are 
> not a class, such as a struct (so the name is a bit of a misnomer, but not 
> too awful), a class template (which you can't match with this interface), or 
> a template type (which you also can't match with this interface).
I don't feel super strongly about this but I also slightly prefer `hasType`.

To be fair - I didn't really have things like inheritance from template 
parameters on my mind when working on `hasAnyBase` (it's definitely not tested) 
so I'd rather not assume it works.



Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:3553
+/// \endcode
+AST_MATCHER_P(CXXBaseSpecifier, hasClass, internal::Matcher,
+  InnerMatcher) {

Nit: while "[base specifier] `hasType`" sounds natural to me for some reason 
`hasClass` doesn't. English is not my first language though.



Comment at: clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp:3158
+class Base {};
+class Derived : BAse {};
+  )cc",

Is this (`Base` != `BAse`) a typo or a way how to tease the asserts?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81552



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

[PATCH] D81552: [ASTMatchers] Added hasDirectBase and hasClass Matchers

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:2888-2890
+///   class Foo;
+///   class Bar : Foo {};
+///   class Baz : Bar {};

njames93 wrote:
> aaron.ballman wrote:
> > It seems like these aren't really part of the example?
> They are, just not directly. Shows it won't match any old base specifier.
Ah, might be good to put comments on there that say `// doesn't match` to make 
it more clear.



Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:3537
 AST_POLYMORPHIC_MATCHER_P_OVERLOAD(
-hasType,
-AST_POLYMORPHIC_SUPPORTED_TYPES(Expr, FriendDecl, ValueDecl,
-CXXBaseSpecifier),
+hasType, AST_POLYMORPHIC_SUPPORTED_TYPES(Expr, FriendDecl, ValueDecl),
 internal::Matcher, InnerMatcher, 1) {

njames93 wrote:
> aaron.ballman wrote:
> > This is undoing a change that was just added less than two weeks ago, so I 
> > think the potential for breaking code is small. That said, can you explain 
> > why you think `hasClass` is a better approach than `hasType`?
> Yeah, as that change hasn't reached landed onto a release branch breaking 
> code shouldn't be an issue, If it was I'd leave it in.
> 
> - `hasType` is very generic, whereas `hasClass` is specific to what a 
> `CXXBaseSpecifier` supports.
> - It makes the matchers marginally simpler.
>   `hasDirectBase(hasType(cxxRecordDecl(hasName("Base"` vs 
> `hasDirectBase(hasClass(hasName("Base")))`
> - In the documentation it also specifies that `hasClass` takes a 
> `Matcher, making it more user friendly.
FWIW, I prefer `hasType` to `hasClass`. You can inherit from things which are 
not a class, such as a struct (so the name is a bit of a misnomer, but not too 
awful), a class template (which you can't match with this interface), or a 
template type (which you also can't match with this interface).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81552



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

[PATCH] D81552: [ASTMatchers] Added hasDirectBase and hasClass Matchers

[Nathan James via Phabricator via cfe-commits]

njames93 marked 2 inline comments as done.
njames93 added inline comments.



Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:2888-2890
+///   class Foo;
+///   class Bar : Foo {};
+///   class Baz : Bar {};

aaron.ballman wrote:
> It seems like these aren't really part of the example?
They are, just not directly. Shows it won't match any old base specifier.



Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:3537
 AST_POLYMORPHIC_MATCHER_P_OVERLOAD(
-hasType,
-AST_POLYMORPHIC_SUPPORTED_TYPES(Expr, FriendDecl, ValueDecl,
-CXXBaseSpecifier),
+hasType, AST_POLYMORPHIC_SUPPORTED_TYPES(Expr, FriendDecl, ValueDecl),
 internal::Matcher, InnerMatcher, 1) {

aaron.ballman wrote:
> This is undoing a change that was just added less than two weeks ago, so I 
> think the potential for breaking code is small. That said, can you explain 
> why you think `hasClass` is a better approach than `hasType`?
Yeah, as that change hasn't reached landed onto a release branch breaking code 
shouldn't be an issue, If it was I'd leave it in.

- `hasType` is very generic, whereas `hasClass` is specific to what a 
`CXXBaseSpecifier` supports.
- It makes the matchers marginally simpler.
  `hasDirectBase(hasType(cxxRecordDecl(hasName("Base"` vs 
`hasDirectBase(hasClass(hasName("Base")))`
- In the documentation it also specifies that `hasClass` takes a 
`Matcher, making it more user friendly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81552



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

[PATCH] D81552: [ASTMatchers] Added hasDirectBase and hasClass Matchers

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:2888-2890
+///   class Foo;
+///   class Bar : Foo {};
+///   class Baz : Bar {};

It seems like these aren't really part of the example?



Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:3537
 AST_POLYMORPHIC_MATCHER_P_OVERLOAD(
-hasType,
-AST_POLYMORPHIC_SUPPORTED_TYPES(Expr, FriendDecl, ValueDecl,
-CXXBaseSpecifier),
+hasType, AST_POLYMORPHIC_SUPPORTED_TYPES(Expr, FriendDecl, ValueDecl),
 internal::Matcher, InnerMatcher, 1) {

This is undoing a change that was just added less than two weeks ago, so I 
think the potential for breaking code is small. That said, can you explain why 
you think `hasClass` is a better approach than `hasType`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81552



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

[PATCH] D81552: [ASTMatchers] Added hasDirectBase and hasClass Matchers

[Nathan James via Phabricator via cfe-commits]

njames93 created this revision.
njames93 added reviewers: klimek, aaron.ballman, jkorous.
Herald added subscribers: cfe-commits, dexonsmith.
Herald added a project: clang.

Adds a matcher called `hasDirectBase` for matching the `CXXBaseSpecifier` of a 
class that directly derives from another class.
Adds a matcher called `hasClass` that matches on the class that a 
`CXXBaseSpecifier` refers to.
Also removed the `CXXBaseSpecifier` overload for the `hasType` Matcher in 
favour of this `hasClass` matcher.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D81552

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

Index: clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
===
--- clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
+++ clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
@@ -2997,26 +2997,24 @@
 }
 
 TEST(HasAnyBase, DirectBase) {
-  EXPECT_TRUE(matches(
-  "struct Base {};"
-  "struct ExpectedMatch : Base {};",
-  cxxRecordDecl(hasName("ExpectedMatch"),
-hasAnyBase(hasType(cxxRecordDecl(hasName("Base")));
+  EXPECT_TRUE(matches("struct Base {};"
+  "struct ExpectedMatch : Base {};",
+  cxxRecordDecl(hasName("ExpectedMatch"),
+hasAnyBase(hasClass(hasName("Base"));
 }
 
 TEST(HasAnyBase, IndirectBase) {
-  EXPECT_TRUE(matches(
-  "struct Base {};"
-  "struct Intermediate : Base {};"
-  "struct ExpectedMatch : Intermediate {};",
-  cxxRecordDecl(hasName("ExpectedMatch"),
-hasAnyBase(hasType(cxxRecordDecl(hasName("Base")));
+  EXPECT_TRUE(matches("struct Base {};"
+  "struct Intermediate : Base {};"
+  "struct ExpectedMatch : Intermediate {};",
+  cxxRecordDecl(hasName("ExpectedMatch"),
+hasAnyBase(hasClass(hasName("Base"));
 }
 
 TEST(HasAnyBase, NoBase) {
   EXPECT_TRUE(notMatches("struct Foo {};"
  "struct Bar {};",
- cxxRecordDecl(hasAnyBase(hasType(cxxRecordDecl());
+ cxxRecordDecl(hasAnyBase(hasClass(cxxRecordDecl());
 }
 
 TEST(IsPublicBase, Public) {
@@ -3117,5 +3115,57 @@
  cxxRecordDecl(hasAnyBase(isVirtual();
 }
 
+TEST(BaseSpecifier, hasDirectBase) {
+  EXPECT_TRUE(matches(
+  R"cc(
+class Base {};
+class Derived : Base{};
+)cc",
+  cxxRecordDecl(hasName("Derived"),
+hasDirectBase(hasClass(hasName("Base"));
+
+  StringRef MultiDerived = R"cc(
+class Base {};
+class Base2 {};
+class Derived : Base, Base2{};
+)cc";
+
+  EXPECT_TRUE(matches(MultiDerived,
+  cxxRecordDecl(hasName("Derived"),
+hasDirectBase(hasClass(hasName("Base"));
+  EXPECT_TRUE(matches(
+  MultiDerived, cxxRecordDecl(hasName("Derived"),
+  hasDirectBase(hasClass(hasName("Base2"));
+
+  StringRef Indirect = R"cc(
+class Base {};
+class Intermediate : Base {};
+class Derived : Intermediate{};
+)cc";
+
+  EXPECT_TRUE(
+  matches(Indirect,
+  cxxRecordDecl(hasName("Derived"),
+hasDirectBase(hasClass(hasName("Intermediate"));
+  EXPECT_TRUE(notMatches(
+  Indirect, cxxRecordDecl(hasName("Derived"),
+  hasDirectBase(hasClass(hasName("Base"));
+
+  // Only really here to make sure the asserts don't fire
+  EXPECT_FALSE(
+  matches(R"cc(
+class Base {};
+class Derived : BAse {};
+  )cc",
+  cxxRecordDecl(hasDirectBase(hasClass(hasName("Base"));
+
+  EXPECT_FALSE(
+  matches(R"cc(
+using Base = int;
+class Derived : Base {};
+  )cc",
+  cxxRecordDecl(hasDirectBase(hasClass(hasName("Base"));
+}
+
 } // namespace ast_matchers
 } // namespace clang
Index: clang/lib/ASTMatchers/Dynamic/Registry.cpp
===
--- clang/lib/ASTMatchers/Dynamic/Registry.cpp
+++ clang/lib/ASTMatchers/Dynamic/Registry.cpp
@@ -261,6 +261,7 @@
   REGISTER_MATCHER(hasCanonicalType);
   REGISTER_MATCHER(hasCaseConstant);
   REGISTER_MATCHER(hasCastKind);
+  REGISTER_MATCHER(hasClass);
   REGISTER_MATCHER(hasCondition);
   REGISTER_MATCHER(hasConditionVariableStatement);
   REGISTER_MATCHER(hasDecayedType);
@@ -271,6 +272,7 @@
   REGISTER_MATCHER(hasDefinition);
   REGISTER_MATCHER(hasDescendant);
   REGISTER_MATCHER(hasDestinationType);
+  REGISTER_MATCHER(hasDirectBase);
   REGISTER_MATCHER(hasDynamicExceptionSpec);
   REGISTER_MATCHER(has