[PATCH] D60543: [clang] Add matcher for subclasses of Objective-C interfaces
stephanemoore updated this revision to Diff 198956.
stephanemoore added a comment.
Present one potential option for making isDerivedFrom support Objective-C
classes.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D60543/new/
https://reviews.llvm.org/D60543
Files:
clang/docs/LibASTMatchersReference.html
clang/include/clang/ASTMatchers/ASTMatchers.h
clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
Index: clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
===
--- clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
+++ clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
@@ -536,6 +536,39 @@
cxxRecordDecl(isDerivedFrom(namedDecl(hasName("X"));
}
+TEST(DeclarationMatcher, ObjCClassIsDerived) {
+ DeclarationMatcher IsDerivedFromX = objcInterfaceDecl(isDerivedFrom("X"));
+ EXPECT_TRUE(matchesObjC("@interface X @end @interface Y : X @end", IsDerivedFromX));
+ EXPECT_TRUE(matchesObjC("@interface X @end @interface Y<__covariant ObjectType> : X @end", IsDerivedFromX));
+ EXPECT_TRUE(notMatchesObjC("@interface X @end", IsDerivedFromX));
+ EXPECT_TRUE(notMatchesObjC("@class X;", IsDerivedFromX));
+ EXPECT_TRUE(notMatchesObjC("@class Y;", IsDerivedFromX));
+
+ DeclarationMatcher IsAX = objcInterfaceDecl(isSameOrDerivedFrom("X"));
+ EXPECT_TRUE(matchesObjC("@interface X @end @interface Y : X @end", IsAX));
+ EXPECT_TRUE(matchesObjC("@interface X @end", IsAX));
+ EXPECT_TRUE(matchesObjC("@class X;", IsAX));
+ EXPECT_TRUE(notMatchesObjC("@interface Y @end", IsAX));
+ EXPECT_TRUE(notMatchesObjC("@class Y;", IsAX));
+
+ DeclarationMatcher ZIsDerivedFromX = objcInterfaceDecl(hasName("Z"), isDerivedFrom("X"));
+ EXPECT_TRUE(
+ matchesObjC("@interface X @end @interface Y : X @end @interface Z : Y @end",
+ ZIsDerivedFromX));
+ EXPECT_TRUE(
+ matchesObjC("@interface X @end typedef X Y; @interface Z : Y @end",
+ ZIsDerivedFromX));
+ EXPECT_TRUE(
+ matchesObjC("@interface X @end @interface Y : X @end typedef Y V; typedef V W; @interface Z : W @end",
+ ZIsDerivedFromX));
+ EXPECT_TRUE(
+ notMatchesObjC("@interface Y @end typedef Y X; @interface Z : X @end",
+ ZIsDerivedFromX));
+ EXPECT_TRUE(
+ notMatchesObjC("@interface Y @end @interface Y2 @end typedef Y2 X; @interface Z : Y @end",
+ ZIsDerivedFromX));
+}
+
TEST(DeclarationMatcher, IsLambda) {
const auto IsLambda = cxxMethodDecl(ofClass(cxxRecordDecl(isLambda(;
EXPECT_TRUE(matches("auto x = []{};", IsLambda));
Index: clang/include/clang/ASTMatchers/ASTMatchers.h
===
--- clang/include/clang/ASTMatchers/ASTMatchers.h
+++ clang/include/clang/ASTMatchers/ASTMatchers.h
@@ -2600,8 +2600,9 @@
AST_POLYMORPHIC_SUPPORTED_TYPES(CXXOperatorCallExpr, FunctionDecl)>(Name);
}
-/// Matches C++ classes that are directly or indirectly derived from
-/// a class matching \c Base.
+/// Matches C++ classes that are directly or indirectly derived from a class
+/// matching \c Base, or Objective-C classes that directly or indirectly
+/// subclass a class matching \c Base.
///
/// Note that a class is not considered to be derived from itself.
///
@@ -2621,28 +2622,49 @@
/// typedef Foo X;
/// class Bar : public Foo {}; // derived from a type that X is a typedef of
/// \endcode
-AST_MATCHER_P(CXXRecordDecl, isDerivedFrom,
+///
+/// In the following example, Bar matches isDerivedFrom(hasName("NSObject"))
+/// \code
+/// @interface NSObject @end
+/// @interface Bar : NSObject @end
+/// \endcode
+AST_MATCHER_P(NamedDecl, isDerivedFrom,
internal::Matcher, Base) {
- return Finder->classIsDerivedFrom(, Base, Builder);
+ // Check if the node is a C++ struct/union/class.
+ if (const auto *RecordDecl = dyn_cast())
+return Finder->classIsDerivedFrom(RecordDecl, Base, Builder);
+
+ // Check if the node is an Objective-C class.
+ if (const auto *InterfaceDecl = dyn_cast()) {
+// Check if any of the superclasses of the class match.
+for (const ObjCInterfaceDecl *SuperClass = InterfaceDecl->getSuperClass();
+ SuperClass != nullptr; SuperClass = SuperClass->getSuperClass()) {
+ if (Base.matches(*SuperClass, Finder, Builder))
+return true;
+}
+ }
+
+ // No matches found.
+ return false;
}
/// Overloaded method as shortcut for \c isDerivedFrom(hasName(...)).
-AST_MATCHER_P_OVERLOAD(CXXRecordDecl, isDerivedFrom, std::string, BaseName, 1) {
+AST_MATCHER_P_OVERLOAD(NamedDecl, isDerivedFrom, std::string, BaseName, 1) {
assert(!BaseName.empty());
return isDerivedFrom(hasName(BaseName)).matches(Node, Finder, Builder);
}
/// Similar to \c isDerivedFrom(), but also matches classes that directly
/// match \c Base.
-AST_MATCHER_P_OVERLOAD(CXXRecordDecl, isSameOrDerivedFrom,
[PATCH] D60543: [clang] Add matcher for subclasses of Objective-C interfaces
aaron.ballman added inline comments. Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:1479 +/// \endcode +AST_MATCHER_P(ObjCInterfaceDecl, isSubclassOfInterface, + internal::Matcher, jordan_rose wrote: > aaron.ballman wrote: > > jordan_rose wrote: > > > aaron.ballman wrote: > > > > jordan_rose wrote: > > > > > aaron.ballman wrote: > > > > > > stephanemoore wrote: > > > > > > > aaron.ballman wrote: > > > > > > > > stephanemoore wrote: > > > > > > > > > I am still uncertain about the naming. > > > > > > > > > > > > > > > > > > `isSubclassOf` seemed too generic as that could apply to C++ > > > > > > > > > classes. > > > > > > > > > `objcIsSubclassOf` seemed unconventional as a matcher name. > > > > > > > > > `isSubclassOfObjCInterface` and `isSubclassOfObjCClass` > > > > > > > > > seemed awkwardly lengthy. > > > > > > > > > Creating a new namespace `clang::ast_matchers::objc` seemed > > > > > > > > > unprecedented. > > > > > > > > > > > > > > > > > > I am happy to change the name if you think another name would > > > > > > > > > be more appropriate. > > > > > > > > Does ObjC use the term "derived" by any chance? We already have > > > > > > > > `isDerivedFrom`, so I'm wondering if we can use that to also > > > > > > > > match on an `ObjCInterfaceDecl`? > > > > > > > Objective-C doesn't generally use the term "derived" (for > > > > > > > example, see archive of [Programming With Objective-C > Defining > > > > > > > Classes](https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/ProgrammingWithObjectiveC/DefiningClasses/DefiningClasses.html#//apple_ref/doc/uid/TP40011210-CH3-SW1)). > > > > > > > With that said, I don't think it's unreasonable or incorrect to > > > > > > > use the term "derived" to describe inheritance in Objective-C. > > > > > > > The behavior of this matcher is also consistent with the behavior > > > > > > > of `isDerivedFrom`. In order to change `isDerivedFrom`, I would > > > > > > > also need to update `isSameOrDerivedFrom`. That would probably be > > > > > > > a good thing so that derivation matching feature set is > > > > > > > consistent for C++ and Objective-C language variants. > > > > > > > > > > > > > > Let me take a crack at merging this into `isDerivedFrom`. > > > > > > I agree that if we go with `isDerivedFrom`, you should update > > > > > > `isSameOrDerivedFrom` at the same time. > > > > > `isSubclassOf` sounds right to me, and since ObjC and C++ class > > > > > hierarchies can't mix, it _should_ be okay, right? They're analogous > > > > > concepts. > > > > > isSubclassOf sounds right to me, and since ObjC and C++ class > > > > > hierarchies can't mix, it _should_ be okay, right? They're analogous > > > > > concepts. > > > > > > > > In the AST matchers, we try to overload the matchers that have similar > > > > behavior. My concern is that a user will reach for `isSubclassOf()` > > > > when they really mean `isDerivedFrom()` or vice versa, and only through > > > > annoying error messages learns about their mistake. Given that we > > > > already have `isDerivedFrom()` and renaming it would break code, I was > > > > trying to determine whether using that name for both C++ derivation and > > > > ObjC derivation would be acceptable. > > > Ah, I see what you mean. Yes, I guess it's more important to be > > > consistent than to perfectly match the terminology. You will certainly > > > confuse an ObjC-only developer at first by using "non-standard" > > > terminology, but any developer has to learn a certain amount of > > > compiler-isms anyway using AST matchers. > > Okay, so it sounds like it wouldn't be hugely problematic to go with > > `isDerivedFrom()`, just that it may sound a bit confusing at first to an > > ObjC developer. Are there some words you think we should add to the > > documentation to help an ObjC developer searching for this functionality? > I think just including "subclass" is sufficient. For example, the current doc > comment is > > ``` > /// Matches C++ classes that are directly or indirectly derived from > /// a class matching \c Base. > ``` > > and it could be changed to something like > > ``` > /// Matches C++ classes that are directly or indirectly derived from > /// a class matching \c Base, or Objective-C classes that directly or > /// indirectly subclass a class matching \c Base. > ``` > > A little clunky, but you get what I mean. > Fantastic, that makes sense to me. Thank you for weighing in! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60543/new/ https://reviews.llvm.org/D60543 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D60543: [clang] Add matcher for subclasses of Objective-C interfaces
jordan_rose added inline comments. Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:1479 +/// \endcode +AST_MATCHER_P(ObjCInterfaceDecl, isSubclassOfInterface, + internal::Matcher, aaron.ballman wrote: > jordan_rose wrote: > > aaron.ballman wrote: > > > jordan_rose wrote: > > > > aaron.ballman wrote: > > > > > stephanemoore wrote: > > > > > > aaron.ballman wrote: > > > > > > > stephanemoore wrote: > > > > > > > > I am still uncertain about the naming. > > > > > > > > > > > > > > > > `isSubclassOf` seemed too generic as that could apply to C++ > > > > > > > > classes. > > > > > > > > `objcIsSubclassOf` seemed unconventional as a matcher name. > > > > > > > > `isSubclassOfObjCInterface` and `isSubclassOfObjCClass` seemed > > > > > > > > awkwardly lengthy. > > > > > > > > Creating a new namespace `clang::ast_matchers::objc` seemed > > > > > > > > unprecedented. > > > > > > > > > > > > > > > > I am happy to change the name if you think another name would > > > > > > > > be more appropriate. > > > > > > > Does ObjC use the term "derived" by any chance? We already have > > > > > > > `isDerivedFrom`, so I'm wondering if we can use that to also > > > > > > > match on an `ObjCInterfaceDecl`? > > > > > > Objective-C doesn't generally use the term "derived" (for example, > > > > > > see archive of [Programming With Objective-C > Defining > > > > > > Classes](https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/ProgrammingWithObjectiveC/DefiningClasses/DefiningClasses.html#//apple_ref/doc/uid/TP40011210-CH3-SW1)). > > > > > > With that said, I don't think it's unreasonable or incorrect to > > > > > > use the term "derived" to describe inheritance in Objective-C. The > > > > > > behavior of this matcher is also consistent with the behavior of > > > > > > `isDerivedFrom`. In order to change `isDerivedFrom`, I would also > > > > > > need to update `isSameOrDerivedFrom`. That would probably be a good > > > > > > thing so that derivation matching feature set is consistent for C++ > > > > > > and Objective-C language variants. > > > > > > > > > > > > Let me take a crack at merging this into `isDerivedFrom`. > > > > > I agree that if we go with `isDerivedFrom`, you should update > > > > > `isSameOrDerivedFrom` at the same time. > > > > `isSubclassOf` sounds right to me, and since ObjC and C++ class > > > > hierarchies can't mix, it _should_ be okay, right? They're analogous > > > > concepts. > > > > isSubclassOf sounds right to me, and since ObjC and C++ class > > > > hierarchies can't mix, it _should_ be okay, right? They're analogous > > > > concepts. > > > > > > In the AST matchers, we try to overload the matchers that have similar > > > behavior. My concern is that a user will reach for `isSubclassOf()` when > > > they really mean `isDerivedFrom()` or vice versa, and only through > > > annoying error messages learns about their mistake. Given that we already > > > have `isDerivedFrom()` and renaming it would break code, I was trying to > > > determine whether using that name for both C++ derivation and ObjC > > > derivation would be acceptable. > > Ah, I see what you mean. Yes, I guess it's more important to be consistent > > than to perfectly match the terminology. You will certainly confuse an > > ObjC-only developer at first by using "non-standard" terminology, but any > > developer has to learn a certain amount of compiler-isms anyway using AST > > matchers. > Okay, so it sounds like it wouldn't be hugely problematic to go with > `isDerivedFrom()`, just that it may sound a bit confusing at first to an ObjC > developer. Are there some words you think we should add to the documentation > to help an ObjC developer searching for this functionality? I think just including "subclass" is sufficient. For example, the current doc comment is ``` /// Matches C++ classes that are directly or indirectly derived from /// a class matching \c Base. ``` and it could be changed to something like ``` /// Matches C++ classes that are directly or indirectly derived from /// a class matching \c Base, or Objective-C classes that directly or /// indirectly subclass a class matching \c Base. ``` A little clunky, but you get what I mean. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60543/new/ https://reviews.llvm.org/D60543 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D60543: [clang] Add matcher for subclasses of Objective-C interfaces
aaron.ballman added inline comments. Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:1479 +/// \endcode +AST_MATCHER_P(ObjCInterfaceDecl, isSubclassOfInterface, + internal::Matcher, jordan_rose wrote: > aaron.ballman wrote: > > jordan_rose wrote: > > > aaron.ballman wrote: > > > > stephanemoore wrote: > > > > > aaron.ballman wrote: > > > > > > stephanemoore wrote: > > > > > > > I am still uncertain about the naming. > > > > > > > > > > > > > > `isSubclassOf` seemed too generic as that could apply to C++ > > > > > > > classes. > > > > > > > `objcIsSubclassOf` seemed unconventional as a matcher name. > > > > > > > `isSubclassOfObjCInterface` and `isSubclassOfObjCClass` seemed > > > > > > > awkwardly lengthy. > > > > > > > Creating a new namespace `clang::ast_matchers::objc` seemed > > > > > > > unprecedented. > > > > > > > > > > > > > > I am happy to change the name if you think another name would be > > > > > > > more appropriate. > > > > > > Does ObjC use the term "derived" by any chance? We already have > > > > > > `isDerivedFrom`, so I'm wondering if we can use that to also match > > > > > > on an `ObjCInterfaceDecl`? > > > > > Objective-C doesn't generally use the term "derived" (for example, > > > > > see archive of [Programming With Objective-C > Defining > > > > > Classes](https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/ProgrammingWithObjectiveC/DefiningClasses/DefiningClasses.html#//apple_ref/doc/uid/TP40011210-CH3-SW1)). > > > > > With that said, I don't think it's unreasonable or incorrect to use > > > > > the term "derived" to describe inheritance in Objective-C. The > > > > > behavior of this matcher is also consistent with the behavior of > > > > > `isDerivedFrom`. In order to change `isDerivedFrom`, I would also > > > > > need to update `isSameOrDerivedFrom`. That would probably be a good > > > > > thing so that derivation matching feature set is consistent for C++ > > > > > and Objective-C language variants. > > > > > > > > > > Let me take a crack at merging this into `isDerivedFrom`. > > > > I agree that if we go with `isDerivedFrom`, you should update > > > > `isSameOrDerivedFrom` at the same time. > > > `isSubclassOf` sounds right to me, and since ObjC and C++ class > > > hierarchies can't mix, it _should_ be okay, right? They're analogous > > > concepts. > > > isSubclassOf sounds right to me, and since ObjC and C++ class hierarchies > > > can't mix, it _should_ be okay, right? They're analogous concepts. > > > > In the AST matchers, we try to overload the matchers that have similar > > behavior. My concern is that a user will reach for `isSubclassOf()` when > > they really mean `isDerivedFrom()` or vice versa, and only through annoying > > error messages learns about their mistake. Given that we already have > > `isDerivedFrom()` and renaming it would break code, I was trying to > > determine whether using that name for both C++ derivation and ObjC > > derivation would be acceptable. > Ah, I see what you mean. Yes, I guess it's more important to be consistent > than to perfectly match the terminology. You will certainly confuse an > ObjC-only developer at first by using "non-standard" terminology, but any > developer has to learn a certain amount of compiler-isms anyway using AST > matchers. Okay, so it sounds like it wouldn't be hugely problematic to go with `isDerivedFrom()`, just that it may sound a bit confusing at first to an ObjC developer. Are there some words you think we should add to the documentation to help an ObjC developer searching for this functionality? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60543/new/ https://reviews.llvm.org/D60543 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D60543: [clang] Add matcher for subclasses of Objective-C interfaces
jordan_rose added inline comments. Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:1479 +/// \endcode +AST_MATCHER_P(ObjCInterfaceDecl, isSubclassOfInterface, + internal::Matcher, aaron.ballman wrote: > jordan_rose wrote: > > aaron.ballman wrote: > > > stephanemoore wrote: > > > > aaron.ballman wrote: > > > > > stephanemoore wrote: > > > > > > I am still uncertain about the naming. > > > > > > > > > > > > `isSubclassOf` seemed too generic as that could apply to C++ > > > > > > classes. > > > > > > `objcIsSubclassOf` seemed unconventional as a matcher name. > > > > > > `isSubclassOfObjCInterface` and `isSubclassOfObjCClass` seemed > > > > > > awkwardly lengthy. > > > > > > Creating a new namespace `clang::ast_matchers::objc` seemed > > > > > > unprecedented. > > > > > > > > > > > > I am happy to change the name if you think another name would be > > > > > > more appropriate. > > > > > Does ObjC use the term "derived" by any chance? We already have > > > > > `isDerivedFrom`, so I'm wondering if we can use that to also match on > > > > > an `ObjCInterfaceDecl`? > > > > Objective-C doesn't generally use the term "derived" (for example, see > > > > archive of [Programming With Objective-C > Defining > > > > Classes](https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/ProgrammingWithObjectiveC/DefiningClasses/DefiningClasses.html#//apple_ref/doc/uid/TP40011210-CH3-SW1)). > > > > With that said, I don't think it's unreasonable or incorrect to use > > > > the term "derived" to describe inheritance in Objective-C. The behavior > > > > of this matcher is also consistent with the behavior of > > > > `isDerivedFrom`. In order to change `isDerivedFrom`, I would also need > > > > to update `isSameOrDerivedFrom`. That would probably be a good thing so > > > > that derivation matching feature set is consistent for C++ and > > > > Objective-C language variants. > > > > > > > > Let me take a crack at merging this into `isDerivedFrom`. > > > I agree that if we go with `isDerivedFrom`, you should update > > > `isSameOrDerivedFrom` at the same time. > > `isSubclassOf` sounds right to me, and since ObjC and C++ class hierarchies > > can't mix, it _should_ be okay, right? They're analogous concepts. > > isSubclassOf sounds right to me, and since ObjC and C++ class hierarchies > > can't mix, it _should_ be okay, right? They're analogous concepts. > > In the AST matchers, we try to overload the matchers that have similar > behavior. My concern is that a user will reach for `isSubclassOf()` when they > really mean `isDerivedFrom()` or vice versa, and only through annoying error > messages learns about their mistake. Given that we already have > `isDerivedFrom()` and renaming it would break code, I was trying to determine > whether using that name for both C++ derivation and ObjC derivation would be > acceptable. Ah, I see what you mean. Yes, I guess it's more important to be consistent than to perfectly match the terminology. You will certainly confuse an ObjC-only developer at first by using "non-standard" terminology, but any developer has to learn a certain amount of compiler-isms anyway using AST matchers. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60543/new/ https://reviews.llvm.org/D60543 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D60543: [clang] Add matcher for subclasses of Objective-C interfaces
aaron.ballman added inline comments. Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:1479 +/// \endcode +AST_MATCHER_P(ObjCInterfaceDecl, isSubclassOfInterface, + internal::Matcher, jordan_rose wrote: > aaron.ballman wrote: > > stephanemoore wrote: > > > aaron.ballman wrote: > > > > stephanemoore wrote: > > > > > I am still uncertain about the naming. > > > > > > > > > > `isSubclassOf` seemed too generic as that could apply to C++ classes. > > > > > `objcIsSubclassOf` seemed unconventional as a matcher name. > > > > > `isSubclassOfObjCInterface` and `isSubclassOfObjCClass` seemed > > > > > awkwardly lengthy. > > > > > Creating a new namespace `clang::ast_matchers::objc` seemed > > > > > unprecedented. > > > > > > > > > > I am happy to change the name if you think another name would be more > > > > > appropriate. > > > > Does ObjC use the term "derived" by any chance? We already have > > > > `isDerivedFrom`, so I'm wondering if we can use that to also match on > > > > an `ObjCInterfaceDecl`? > > > Objective-C doesn't generally use the term "derived" (for example, see > > > archive of [Programming With Objective-C > Defining > > > Classes](https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/ProgrammingWithObjectiveC/DefiningClasses/DefiningClasses.html#//apple_ref/doc/uid/TP40011210-CH3-SW1)). > > > With that said, I don't think it's unreasonable or incorrect to use the > > > term "derived" to describe inheritance in Objective-C. The behavior of > > > this matcher is also consistent with the behavior of `isDerivedFrom`. In > > > order to change `isDerivedFrom`, I would also need to update > > > `isSameOrDerivedFrom`. That would probably be a good thing so that > > > derivation matching feature set is consistent for C++ and Objective-C > > > language variants. > > > > > > Let me take a crack at merging this into `isDerivedFrom`. > > I agree that if we go with `isDerivedFrom`, you should update > > `isSameOrDerivedFrom` at the same time. > `isSubclassOf` sounds right to me, and since ObjC and C++ class hierarchies > can't mix, it _should_ be okay, right? They're analogous concepts. > isSubclassOf sounds right to me, and since ObjC and C++ class hierarchies > can't mix, it _should_ be okay, right? They're analogous concepts. In the AST matchers, we try to overload the matchers that have similar behavior. My concern is that a user will reach for `isSubclassOf()` when they really mean `isDerivedFrom()` or vice versa, and only through annoying error messages learns about their mistake. Given that we already have `isDerivedFrom()` and renaming it would break code, I was trying to determine whether using that name for both C++ derivation and ObjC derivation would be acceptable. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60543/new/ https://reviews.llvm.org/D60543 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D60543: [clang] Add matcher for subclasses of Objective-C interfaces
jordan_rose added inline comments. Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:1479 +/// \endcode +AST_MATCHER_P(ObjCInterfaceDecl, isSubclassOfInterface, + internal::Matcher, aaron.ballman wrote: > stephanemoore wrote: > > aaron.ballman wrote: > > > stephanemoore wrote: > > > > I am still uncertain about the naming. > > > > > > > > `isSubclassOf` seemed too generic as that could apply to C++ classes. > > > > `objcIsSubclassOf` seemed unconventional as a matcher name. > > > > `isSubclassOfObjCInterface` and `isSubclassOfObjCClass` seemed > > > > awkwardly lengthy. > > > > Creating a new namespace `clang::ast_matchers::objc` seemed > > > > unprecedented. > > > > > > > > I am happy to change the name if you think another name would be more > > > > appropriate. > > > Does ObjC use the term "derived" by any chance? We already have > > > `isDerivedFrom`, so I'm wondering if we can use that to also match on an > > > `ObjCInterfaceDecl`? > > Objective-C doesn't generally use the term "derived" (for example, see > > archive of [Programming With Objective-C > Defining > > Classes](https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/ProgrammingWithObjectiveC/DefiningClasses/DefiningClasses.html#//apple_ref/doc/uid/TP40011210-CH3-SW1)). > > With that said, I don't think it's unreasonable or incorrect to use the > > term "derived" to describe inheritance in Objective-C. The behavior of this > > matcher is also consistent with the behavior of `isDerivedFrom`. In order > > to change `isDerivedFrom`, I would also need to update > > `isSameOrDerivedFrom`. That would probably be a good thing so that > > derivation matching feature set is consistent for C++ and Objective-C > > language variants. > > > > Let me take a crack at merging this into `isDerivedFrom`. > I agree that if we go with `isDerivedFrom`, you should update > `isSameOrDerivedFrom` at the same time. `isSubclassOf` sounds right to me, and since ObjC and C++ class hierarchies can't mix, it _should_ be okay, right? They're analogous concepts. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60543/new/ https://reviews.llvm.org/D60543 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D60543: [clang] Add matcher for subclasses of Objective-C interfaces
aaron.ballman added reviewers: jordan_rose, rjmccall. aaron.ballman added a comment. Adding some ObjC experts in case they'd like to weigh in on the name `isDerivedFrom` in this context. Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:1479 +/// \endcode +AST_MATCHER_P(ObjCInterfaceDecl, isSubclassOfInterface, + internal::Matcher, stephanemoore wrote: > aaron.ballman wrote: > > stephanemoore wrote: > > > I am still uncertain about the naming. > > > > > > `isSubclassOf` seemed too generic as that could apply to C++ classes. > > > `objcIsSubclassOf` seemed unconventional as a matcher name. > > > `isSubclassOfObjCInterface` and `isSubclassOfObjCClass` seemed awkwardly > > > lengthy. > > > Creating a new namespace `clang::ast_matchers::objc` seemed unprecedented. > > > > > > I am happy to change the name if you think another name would be more > > > appropriate. > > Does ObjC use the term "derived" by any chance? We already have > > `isDerivedFrom`, so I'm wondering if we can use that to also match on an > > `ObjCInterfaceDecl`? > Objective-C doesn't generally use the term "derived" (for example, see > archive of [Programming With Objective-C > Defining > Classes](https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/ProgrammingWithObjectiveC/DefiningClasses/DefiningClasses.html#//apple_ref/doc/uid/TP40011210-CH3-SW1)). > With that said, I don't think it's unreasonable or incorrect to use the term > "derived" to describe inheritance in Objective-C. The behavior of this > matcher is also consistent with the behavior of `isDerivedFrom`. In order to > change `isDerivedFrom`, I would also need to update `isSameOrDerivedFrom`. > That would probably be a good thing so that derivation matching feature set > is consistent for C++ and Objective-C language variants. > > Let me take a crack at merging this into `isDerivedFrom`. I agree that if we go with `isDerivedFrom`, you should update `isSameOrDerivedFrom` at the same time. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60543/new/ https://reviews.llvm.org/D60543 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D60543: [clang] Add matcher for subclasses of Objective-C interfaces
stephanemoore planned changes to this revision. stephanemoore added inline comments. Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:1479 +/// \endcode +AST_MATCHER_P(ObjCInterfaceDecl, isSubclassOfInterface, + internal::Matcher, aaron.ballman wrote: > stephanemoore wrote: > > I am still uncertain about the naming. > > > > `isSubclassOf` seemed too generic as that could apply to C++ classes. > > `objcIsSubclassOf` seemed unconventional as a matcher name. > > `isSubclassOfObjCInterface` and `isSubclassOfObjCClass` seemed awkwardly > > lengthy. > > Creating a new namespace `clang::ast_matchers::objc` seemed unprecedented. > > > > I am happy to change the name if you think another name would be more > > appropriate. > Does ObjC use the term "derived" by any chance? We already have > `isDerivedFrom`, so I'm wondering if we can use that to also match on an > `ObjCInterfaceDecl`? Objective-C doesn't generally use the term "derived" (for example, see archive of [Programming With Objective-C > Defining Classes](https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/ProgrammingWithObjectiveC/DefiningClasses/DefiningClasses.html#//apple_ref/doc/uid/TP40011210-CH3-SW1)). With that said, I don't think it's unreasonable or incorrect to use the term "derived" to describe inheritance in Objective-C. The behavior of this matcher is also consistent with the behavior of `isDerivedFrom`. In order to change `isDerivedFrom`, I would also need to update `isSameOrDerivedFrom`. That would probably be a good thing so that derivation matching feature set is consistent for C++ and Objective-C language variants. Let me take a crack at merging this into `isDerivedFrom`. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60543/new/ https://reviews.llvm.org/D60543 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D60543: [clang] Add matcher for subclasses of Objective-C interfaces
aaron.ballman added inline comments. Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:1479 +/// \endcode +AST_MATCHER_P(ObjCInterfaceDecl, isSubclassOfInterface, + internal::Matcher, stephanemoore wrote: > I am still uncertain about the naming. > > `isSubclassOf` seemed too generic as that could apply to C++ classes. > `objcIsSubclassOf` seemed unconventional as a matcher name. > `isSubclassOfObjCInterface` and `isSubclassOfObjCClass` seemed awkwardly > lengthy. > Creating a new namespace `clang::ast_matchers::objc` seemed unprecedented. > > I am happy to change the name if you think another name would be more > appropriate. Does ObjC use the term "derived" by any chance? We already have `isDerivedFrom`, so I'm wondering if we can use that to also match on an `ObjCInterfaceDecl`? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60543/new/ https://reviews.llvm.org/D60543 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D60543: [clang] Add matcher for subclasses of Objective-C interfaces
stephanemoore marked an inline comment as done. stephanemoore added inline comments. Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:1479 +/// \endcode +AST_MATCHER_P(ObjCInterfaceDecl, isSubclassOfInterface, + internal::Matcher, I am still uncertain about the naming. `isSubclassOf` seemed too generic as that could apply to C++ classes. `objcIsSubclassOf` seemed unconventional as a matcher name. `isSubclassOfObjCInterface` and `isSubclassOfObjCClass` seemed awkwardly lengthy. Creating a new namespace `clang::ast_matchers::objc` seemed unprecedented. I am happy to change the name if you think another name would be more appropriate. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60543/new/ https://reviews.llvm.org/D60543 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D60543: [clang] Add matcher for subclasses of Objective-C interfaces
stephanemoore created this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
stephanemoore added a reviewer: aaron.ballman.
This change adds a new AST matcher to detect Objective-C classes that
are subclasses of matching Objective-C interfaces.
Test Notes:
Ran clang unit tests.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D60543
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
@@ -1389,6 +1389,23 @@
EXPECT_TRUE(matchesObjC("void f() { ^{}(); }", blockExpr()));
}
+TEST(IsSubclassOfInterfaceMatcher, SubclassMatching) {
+ std::string ObjCString = "@interface A @end"
+ "@interface B : A @end"
+ "@interface C : B @end";
+ EXPECT_TRUE(matchesObjC(
+ ObjCString, objcInterfaceDecl(isSubclassOfInterface(hasName("A");
+ EXPECT_TRUE(matchesObjC(ObjCString,
+ objcInterfaceDecl(isSubclassOfInterface(hasName("A")),
+unless(hasName("B");
+ EXPECT_TRUE(matchesObjC(
+ ObjCString, objcInterfaceDecl(isSubclassOfInterface(hasName("B");
+ EXPECT_FALSE(matchesObjC(
+ ObjCString, objcInterfaceDecl(isSubclassOfInterface(hasName("C");
+ EXPECT_FALSE(matchesObjC(
+ ObjCString, objcInterfaceDecl(isSubclassOfInterface(hasName("X");
+}
+
TEST(StatementCountIs, FindsNoStatementsInAnEmptyCompoundStatement) {
EXPECT_TRUE(matches("void f() { }",
compoundStmt(statementCountIs(0;
Index: clang/lib/ASTMatchers/Dynamic/Registry.cpp
===
--- clang/lib/ASTMatchers/Dynamic/Registry.cpp
+++ clang/lib/ASTMatchers/Dynamic/Registry.cpp
@@ -393,6 +393,7 @@
REGISTER_MATCHER(isStaticLocal);
REGISTER_MATCHER(isStaticStorageClass);
REGISTER_MATCHER(isStruct);
+ REGISTER_MATCHER(isSubclassOfInterface);
REGISTER_MATCHER(isTemplateInstantiation);
REGISTER_MATCHER(isTypeDependent);
REGISTER_MATCHER(isUnion);
Index: clang/include/clang/ASTMatchers/ASTMatchers.h
===
--- clang/include/clang/ASTMatchers/ASTMatchers.h
+++ clang/include/clang/ASTMatchers/ASTMatchers.h
@@ -1461,6 +1461,35 @@
extern const internal::VariadicDynCastAllOfMatcher
objcFinallyStmt;
+/// Matches Objective-C classes that directly or indirectly subclass
+/// a matching superclass.
+///
+/// Note that a class is not considered to be a subclass of itself.
+///
+/// Example matches implementation declarations for Y and Z.
+/// (matcher = objcInterfaceDecl(isSubclassOfInterface(hasName("X"
+/// \code
+/// @interface X
+/// @end
+/// @interface Y : X // directly derived
+/// @end
+/// @interface Z : Y // indirectly derived
+/// @end
+/// \endcode
+AST_MATCHER_P(ObjCInterfaceDecl, isSubclassOfInterface,
+ internal::Matcher,
+ InnerMatcher) {
+ // Check if any of the superclasses of the class match.
+ for (const ObjCInterfaceDecl *SuperClass = Node.getSuperClass();
+ SuperClass != nullptr; SuperClass = SuperClass->getSuperClass()) {
+if (InnerMatcher.matches(*SuperClass, Finder, Builder))
+ return true;
+ }
+
+ // No matches found.
+ return false;
+}
+
/// Matches expressions that introduce cleanups to be run at the end
/// of the sub-expression's evaluation.
///
Index: clang/docs/LibASTMatchersReference.html
===
--- clang/docs/LibASTMatchersReference.html
+++ clang/docs/LibASTMatchersReference.html
@@ -6285,6 +6285,23 @@
+Matcherhttps://clang.llvm.org/doxygen/classclang_1_1ObjCInterfaceDecl.html;>ObjCInterfaceDeclisSubclassOfInterfaceMatcherhttps://clang.llvm.org/doxygen/classclang_1_1ObjCInterfaceDecl.html;>ObjCInterfaceDecl InnerMatcher
+Matches Objective-C classes that directly or indirectly subclass
+a matching superclass.
+
+Note that a class is not considered to be a subclass of itself.
+
+Example matches implementation declarations for Y and Z.
+ (matcher = objcInterfaceDecl(isSubclassOfInterface(hasName("X"
+ @interface X
+ @end
+ @interface Y : X // directly derived
+ @end
+ @interface Z : Y // indirectly derived
+ @end
+
+
+
Matcherhttps://clang.llvm.org/doxygen/classclang_1_1ObjCMessageExpr.html;>ObjCMessageExprhasAnyArgumentMatcherhttps://clang.llvm.org/doxygen/classclang_1_1Expr.html;>Expr InnerMatcher
Matches any argument of a call expression or a constructor call
expression, or an ObjC-message-send expression.
