[PATCH] D60543: [clang] Update isDerivedFrom to support Objective-C classes 🔍

[Stephane Moore via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
stephanemoore marked an inline comment as done.
Closed by commit rL368632: [clang] Update isDerivedFrom to support Objective-C 
classes 🔍 (authored by stephanemoore, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D60543?vs=214272&id=214735#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D60543

Files:
  cfe/trunk/docs/LibASTMatchersReference.html
  cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h
  cfe/trunk/include/clang/ASTMatchers/ASTMatchersInternal.h
  cfe/trunk/lib/ASTMatchers/ASTMatchFinder.cpp
  cfe/trunk/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
  cfe/trunk/unittests/ASTMatchers/Dynamic/RegistryTest.cpp

Index: cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h
===
--- cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h
+++ cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h
@@ -2611,8 +2611,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.
 ///
@@ -2632,36 +2633,80 @@
 ///   typedef Foo X;
 ///   class Bar : public Foo {};  // derived from a type that X is a typedef of
 /// \endcode
-AST_MATCHER_P(CXXRecordDecl, isDerivedFrom,
-  internal::Matcher, Base) {
-  return Finder->classIsDerivedFrom(&Node, Base, Builder, /*Directly=*/false);
+///
+/// In the following example, Bar matches isDerivedFrom(hasName("NSObject"))
+/// \code
+///   @interface NSObject @end
+///   @interface Bar : NSObject @end
+/// \endcode
+///
+/// Usable as: Matcher, Matcher
+AST_POLYMORPHIC_MATCHER_P(
+isDerivedFrom,
+AST_POLYMORPHIC_SUPPORTED_TYPES(CXXRecordDecl, ObjCInterfaceDecl),
+internal::Matcher, Base) {
+  // Check if the node is a C++ struct/union/class.
+  if (const auto *RD = dyn_cast(&Node))
+return Finder->classIsDerivedFrom(RD, Base, Builder, /*Directly=*/false);
+
+  // The node must be an Objective-C class.
+  const auto *InterfaceDecl = cast(&Node);
+  return Finder->objcClassIsDerivedFrom(InterfaceDecl, Base, Builder,
+/*Directly=*/false);
 }
 
 /// Overloaded method as shortcut for \c isDerivedFrom(hasName(...)).
-AST_MATCHER_P_OVERLOAD(CXXRecordDecl, isDerivedFrom, std::string, BaseName, 1) {
+AST_POLYMORPHIC_MATCHER_P_OVERLOAD(
+isDerivedFrom,
+AST_POLYMORPHIC_SUPPORTED_TYPES(CXXRecordDecl, ObjCInterfaceDecl),
+std::string, BaseName, 1) {
   if (BaseName.empty())
 return false;
-  return isDerivedFrom(hasName(BaseName)).matches(Node, Finder, Builder);
+
+  const auto M = isDerivedFrom(hasName(BaseName));
+
+  if (const auto *RD = dyn_cast(&Node))
+return Matcher(M).matches(*RD, Finder, Builder);
+
+  const auto *InterfaceDecl = cast(&Node);
+  return Matcher(M).matches(*InterfaceDecl, Finder, Builder);
 }
 
 /// Similar to \c isDerivedFrom(), but also matches classes that directly
 /// match \c Base.
-AST_MATCHER_P_OVERLOAD(CXXRecordDecl, isSameOrDerivedFrom,
-   internal::Matcher, Base, 0) {
-  return Matcher(anyOf(Base, isDerivedFrom(Base)))
-  .matches(Node, Finder, Builder);
+AST_POLYMORPHIC_MATCHER_P_OVERLOAD(
+isSameOrDerivedFrom,
+AST_POLYMORPHIC_SUPPORTED_TYPES(CXXRecordDecl, ObjCInterfaceDecl),
+internal::Matcher, Base, 0) {
+  const auto M = anyOf(Base, isDerivedFrom(Base));
+
+  if (const auto *RD = dyn_cast(&Node))
+return Matcher(M).matches(*RD, Finder, Builder);
+
+  const auto *InterfaceDecl = cast(&Node);
+  return Matcher(M).matches(*InterfaceDecl, Finder, Builder);
 }
 
 /// Overloaded method as shortcut for
 /// \c isSameOrDerivedFrom(hasName(...)).
-AST_MATCHER_P_OVERLOAD(CXXRecordDecl, isSameOrDerivedFrom, std::string,
-   BaseName, 1) {
+AST_POLYMORPHIC_MATCHER_P_OVERLOAD(
+isSameOrDerivedFrom,
+AST_POLYMORPHIC_SUPPORTED_TYPES(CXXRecordDecl, ObjCInterfaceDecl),
+std::string, BaseName, 1) {
   if (BaseName.empty())
 return false;
-  return isSameOrDerivedFrom(hasName(BaseName)).matches(Node, Finder, Builder);
+
+  const auto M = isSameOrDerivedFrom(hasName(BaseName));
+
+  if (const auto *RD = dyn_cast(&Node))
+return Matcher(M).matches(*RD, Finder, Builder);
+
+  const auto *InterfaceDecl = cast(&Node);
+  return Matcher(M).matches(*InterfaceDecl, Finder, Builder);
 }
 
-/// Matches C++ classes that are directly derived from a class matching \c Base.
+/// Matches C++ or Objective-C classes that are directly derived 

[PATCH] D60543: [clang] Update isDerivedFrom to support Objective-C classes 🔍

[Stephane Moore via Phabricator via cfe-commits]

stephanemoore marked 3 inline comments as done.
stephanemoore added inline comments.



Comment at: clang/lib/ASTMatchers/ASTMatchFinder.cpp:820
+  llvm::DenseMap>
+  CompatibleAliases;

aaron.ballman wrote:
> gribozavr wrote:
> > `unordered_set`?
> or `SmallPtrSet`?
I went with `llvm::SmallPtrSet` as that seems more common in clang sources 
compared to `std::unordered_set`.


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] Update isDerivedFrom to support Objective-C classes 🔍

[Stephane Moore via Phabricator via cfe-commits]

stephanemoore updated this revision to Diff 214272.
stephanemoore added a comment.

Use `llvm::SmallPtrSet` to store the compatible aliases instead of `std::set`.
Fix a stray unit test failure in `RegistryTest.cpp`.


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/include/clang/ASTMatchers/ASTMatchersInternal.h
  clang/lib/ASTMatchers/ASTMatchFinder.cpp
  clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
  clang/unittests/ASTMatchers/Dynamic/RegistryTest.cpp

Index: clang/unittests/ASTMatchers/Dynamic/RegistryTest.cpp
===
--- clang/unittests/ASTMatchers/Dynamic/RegistryTest.cpp
+++ clang/unittests/ASTMatchers/Dynamic/RegistryTest.cpp
@@ -439,7 +439,7 @@
   Error.get()).isNull());
   EXPECT_EQ("Incorrect type for arg 1. "
 "(Expected = Matcher) != "
-"(Actual = Matcher&Matcher"
+"(Actual = Matcher&Matcher"
 ")",
 Error->toString());
 }
Index: clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
===
--- clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
+++ clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
@@ -573,6 +573,111 @@
   EXPECT_TRUE(notMatches(Code, cxxRecordDecl(isSameOrDerivedFrom("";
 }
 
+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(matchesObjC(
+  "@interface X @end @compatibility_alias Y X; @interface Z : Y @end",
+  IsDerivedFromX));
+  EXPECT_TRUE(matchesObjC(
+  "@interface X @end typedef X Y; @interface Z : Y @end", IsDerivedFromX));
+  EXPECT_TRUE(notMatchesObjC("@interface X @end", IsDerivedFromX));
+  EXPECT_TRUE(notMatchesObjC("@class X;", IsDerivedFromX));
+  EXPECT_TRUE(notMatchesObjC("@class Y;", IsDerivedFromX));
+  EXPECT_TRUE(notMatchesObjC("@interface X @end @compatibility_alias Y X;",
+ IsDerivedFromX));
+  EXPECT_TRUE(notMatchesObjC("@interface X @end typedef X Y;", IsDerivedFromX));
+
+  DeclarationMatcher IsDirectlyDerivedFromX =
+  objcInterfaceDecl(isDirectlyDerivedFrom("X"));
+  EXPECT_TRUE(
+  matchesObjC("@interface X @end @interface Y : X @end", IsDirectlyDerivedFromX));
+  EXPECT_TRUE(matchesObjC(
+  "@interface X @end @interface Y<__covariant ObjectType> : X @end",
+  IsDirectlyDerivedFromX));
+  EXPECT_TRUE(matchesObjC(
+  "@interface X @end @compatibility_alias Y X; @interface Z : Y @end",
+  IsDirectlyDerivedFromX));
+  EXPECT_TRUE(matchesObjC(
+  "@interface X @end typedef X Y; @interface Z : Y @end",
+  IsDirectlyDerivedFromX));
+  EXPECT_TRUE(notMatchesObjC("@interface X @end", IsDirectlyDerivedFromX));
+  EXPECT_TRUE(notMatchesObjC("@class X;", IsDirectlyDerivedFromX));
+  EXPECT_TRUE(notMatchesObjC("@class Y;", IsDirectlyDerivedFromX));
+  EXPECT_TRUE(notMatchesObjC("@interface X @end @compatibility_alias Y X;",
+ IsDirectlyDerivedFromX));
+  EXPECT_TRUE(notMatchesObjC("@interface X @end typedef X Y;",
+ IsDirectlyDerivedFromX));
+
+  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"));
+  DeclarationMatcher ZIsDirectlyDerivedFromX =
+  objcInterfaceDecl(hasName("Z"), isDirectlyDerivedFrom("X"));
+  EXPECT_TRUE(matchesObjC(
+  "@interface X @end @interface Y : X @end @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(matchesObjC(
+  "@interface X @end typedef X Y; @interface Z : Y @end", ZIsDerivedFromX));
+  EXPECT_TRUE(matchesObjC(
+  "@interface X @end typedef X Y; @interface Z : Y @end",
+  ZIsDirectlyDerivedFromX));
+  EXPECT_TRUE(matchesObjC(
+  "@interface A @end typedef A X; typedef A Y; @interface Z : Y @end",
+  ZIsDerivedFromX));
+  EXPECT_TRUE(matchesObjC(
+  "@interface A @end typedef A X; typedef A Y; @interface Z : Y @end",
+  ZIsDirectlyDerivedFromX));
+

[PATCH] D60543: [clang] Update isDerivedFrom to support Objective-C classes 🔍

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.

LGTM




Comment at: clang/lib/ASTMatchers/ASTMatchFinder.cpp:820
+  llvm::DenseMap>
+  CompatibleAliases;

gribozavr wrote:
> `unordered_set`?
or `SmallPtrSet`?


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] Update isDerivedFrom to support Objective-C classes 🔍

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr accepted this revision.
gribozavr added inline comments.
This revision is now accepted and ready to land.



Comment at: clang/lib/ASTMatchers/ASTMatchFinder.cpp:820
+  llvm::DenseMap>
+  CompatibleAliases;

`unordered_set`?


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] Update isDerivedFrom to support Objective-C classes 🔍

[Stephane Moore via Phabricator via cfe-commits]

stephanemoore updated this revision to Diff 214053.
stephanemoore added a comment.

Add tests for `isDirectlyDerivedFrom`.


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/include/clang/ASTMatchers/ASTMatchersInternal.h
  clang/lib/ASTMatchers/ASTMatchFinder.cpp
  clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp

Index: clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
===
--- clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
+++ clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
@@ -573,6 +573,111 @@
   EXPECT_TRUE(notMatches(Code, cxxRecordDecl(isSameOrDerivedFrom("";
 }
 
+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(matchesObjC(
+  "@interface X @end @compatibility_alias Y X; @interface Z : Y @end",
+  IsDerivedFromX));
+  EXPECT_TRUE(matchesObjC(
+  "@interface X @end typedef X Y; @interface Z : Y @end", IsDerivedFromX));
+  EXPECT_TRUE(notMatchesObjC("@interface X @end", IsDerivedFromX));
+  EXPECT_TRUE(notMatchesObjC("@class X;", IsDerivedFromX));
+  EXPECT_TRUE(notMatchesObjC("@class Y;", IsDerivedFromX));
+  EXPECT_TRUE(notMatchesObjC("@interface X @end @compatibility_alias Y X;",
+ IsDerivedFromX));
+  EXPECT_TRUE(notMatchesObjC("@interface X @end typedef X Y;", IsDerivedFromX));
+
+  DeclarationMatcher IsDirectlyDerivedFromX =
+  objcInterfaceDecl(isDirectlyDerivedFrom("X"));
+  EXPECT_TRUE(
+  matchesObjC("@interface X @end @interface Y : X @end", IsDirectlyDerivedFromX));
+  EXPECT_TRUE(matchesObjC(
+  "@interface X @end @interface Y<__covariant ObjectType> : X @end",
+  IsDirectlyDerivedFromX));
+  EXPECT_TRUE(matchesObjC(
+  "@interface X @end @compatibility_alias Y X; @interface Z : Y @end",
+  IsDirectlyDerivedFromX));
+  EXPECT_TRUE(matchesObjC(
+  "@interface X @end typedef X Y; @interface Z : Y @end",
+  IsDirectlyDerivedFromX));
+  EXPECT_TRUE(notMatchesObjC("@interface X @end", IsDirectlyDerivedFromX));
+  EXPECT_TRUE(notMatchesObjC("@class X;", IsDirectlyDerivedFromX));
+  EXPECT_TRUE(notMatchesObjC("@class Y;", IsDirectlyDerivedFromX));
+  EXPECT_TRUE(notMatchesObjC("@interface X @end @compatibility_alias Y X;",
+ IsDirectlyDerivedFromX));
+  EXPECT_TRUE(notMatchesObjC("@interface X @end typedef X Y;",
+ IsDirectlyDerivedFromX));
+
+  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"));
+  DeclarationMatcher ZIsDirectlyDerivedFromX =
+  objcInterfaceDecl(hasName("Z"), isDirectlyDerivedFrom("X"));
+  EXPECT_TRUE(matchesObjC(
+  "@interface X @end @interface Y : X @end @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(matchesObjC(
+  "@interface X @end typedef X Y; @interface Z : Y @end", ZIsDerivedFromX));
+  EXPECT_TRUE(matchesObjC(
+  "@interface X @end typedef X Y; @interface Z : Y @end",
+  ZIsDirectlyDerivedFromX));
+  EXPECT_TRUE(matchesObjC(
+  "@interface A @end typedef A X; typedef A Y; @interface Z : Y @end",
+  ZIsDerivedFromX));
+  EXPECT_TRUE(matchesObjC(
+  "@interface A @end typedef A X; typedef A Y; @interface Z : Y @end",
+  ZIsDirectlyDerivedFromX));
+  EXPECT_TRUE(matchesObjC(
+  "@interface X @end @compatibility_alias Y X; @interface Z : Y @end",
+  ZIsDerivedFromX));
+  EXPECT_TRUE(matchesObjC(
+  "@interface X @end @compatibility_alias Y X; @interface Z : Y @end",
+  ZIsDirectlyDerivedFromX));
+  EXPECT_TRUE(matchesObjC(
+  "@interface Y @end @compatibility_alias X Y; @interface Z : Y @end",
+  ZIsDerivedFromX));
+  EXPECT_TRUE(matchesObjC(
+  "@interface Y @end @compatibility_alias X Y; @interface Z : Y @end",
+  ZIsDirectlyDerivedFromX));
+  EXPECT_TRUE(matchesObjC(
+  "@interface A @end @compatibility_alias X A; @compatibility_alias Y A;"
+  "@interfac

[PATCH] D60543: [clang] Update isDerivedFrom to support Objective-C classes 🔍

[Stephane Moore via Phabricator via cfe-commits]

stephanemoore planned changes to this revision.
stephanemoore added a comment.

Whoops; forgot to add test cases for `isDirectlyDerivedFrom` 🤦 Will do that 
shortly.


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] Update isDerivedFrom to support Objective-C classes 🔍

[Stephane Moore via Phabricator via cfe-commits]

stephanemoore added a comment.

I spent some time becoming familiar with how `isDerivedFrom` behaves for C++ 
classes. I //think// that I have managed to get the behavior for Objective-C 
classes to mirror that of C++ classes. Please let me know if I overlooked 
anything.




Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:2642-2649
+  if (const auto *InterfaceDecl = dyn_cast(&Node)) {
+// 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;
+}

stephanemoore wrote:
> aaron.ballman wrote:
> > This should probably be done similar to how `classIsDerivedFrom()` works. 
> > For instance, there's some type alias matching logic that this does not 
> > replicate, but we probably want.
> Upon first glance I had determined that the type aliasing logic in 
> `classIsDerivedFrom()` was particular to C++. On second consideration, we 
> will probably want to make sure that compatibility aliases are handled 
> correctly for Objective-C. I will take a look into making sure that works as 
> expected.
Done.



Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:2667-2672
+  if (const auto *InterfaceDecl = dyn_cast(&Node)) {
+return Matcher(M).matches(*InterfaceDecl, Finder,
+ Builder);
+  }
+
+  llvm_unreachable("Not a valid polymorphic type");

aaron.ballman wrote:
> How about:
> ```
> const auto *InterfaceDecl = cast(&Node);
> return Matcher...
> ```
> We can rely on `cast<>` to assert if the node is of an unexpected type. Same 
> suggestions below.
Good call. Done.


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] Update isDerivedFrom to support Objective-C classes 🔍

[Stephane Moore via Phabricator via cfe-commits]

stephanemoore updated this revision to Diff 214035.
stephanemoore marked 6 inline comments as done.
stephanemoore edited the summary of this revision.
stephanemoore added a comment.

Update `isDerivedFrom` to match aliased types and compatibility aliases of
derived 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/include/clang/ASTMatchers/ASTMatchersInternal.h
  clang/lib/ASTMatchers/ASTMatchFinder.cpp
  clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp

Index: clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
===
--- clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
+++ clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
@@ -573,6 +573,64 @@
   EXPECT_TRUE(notMatches(Code, cxxRecordDecl(isSameOrDerivedFrom("";
 }
 
+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(matchesObjC(
+  "@interface X @end @compatibility_alias Y X; @interface Z : Y @end",
+  IsDerivedFromX));
+  EXPECT_TRUE(matchesObjC(
+  "@interface X @end typedef X Y; @interface Z : Y @end", IsDerivedFromX));
+  EXPECT_TRUE(notMatchesObjC("@interface X @end", IsDerivedFromX));
+  EXPECT_TRUE(notMatchesObjC("@class X;", IsDerivedFromX));
+  EXPECT_TRUE(notMatchesObjC("@class Y;", IsDerivedFromX));
+  EXPECT_TRUE(notMatchesObjC("@interface X @end @compatibility_alias Y X;",
+ IsDerivedFromX));
+  EXPECT_TRUE(notMatchesObjC("@interface X @end typedef X 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 @interface Y : X @end typedef Y "
+  "V; typedef V W; @interface Z : W @end",
+  ZIsDerivedFromX));
+  EXPECT_TRUE(matchesObjC(
+  "@interface X @end typedef X Y; @interface Z : Y @end", ZIsDerivedFromX));
+  EXPECT_TRUE(matchesObjC(
+  "@interface A @end typedef A X; typedef A Y; @interface Z : Y @end",
+  ZIsDerivedFromX));
+  EXPECT_TRUE(matchesObjC(
+  "@interface X @end @compatibility_alias Y X; @interface Z : Y @end",
+  ZIsDerivedFromX));
+  EXPECT_TRUE(matchesObjC(
+  "@interface Y @end @compatibility_alias X Y; @interface Z : Y @end",
+  ZIsDerivedFromX));
+  EXPECT_TRUE(matchesObjC(
+  "@interface A @end @compatibility_alias X A; @compatibility_alias Y A;"
+  "@interface Z : Y @end", ZIsDerivedFromX));
+  EXPECT_TRUE(matchesObjC(
+  "@interface Y @end typedef Y X; @interface Z : X @end", ZIsDerivedFromX));
+  EXPECT_TRUE(matchesObjC(
+  "@interface A @end @compatibility_alias Y A; typedef Y X;"
+  "@interface Z : A @end", ZIsDerivedFromX));
+  EXPECT_TRUE(matchesObjC(
+  "@interface A @end typedef A Y; @compatibility_alias X Y;"
+  "@interface Z : A @end", ZIsDerivedFromX));
+}
+
 TEST(DeclarationMatcher, IsLambda) {
   const auto IsLambda = cxxMethodDecl(ofClass(cxxRecordDecl(isLambda(;
   EXPECT_TRUE(matches("auto x = []{};", IsLambda));
Index: clang/lib/ASTMatchers/ASTMatchFinder.cpp
===
--- clang/lib/ASTMatchers/ASTMatchFinder.cpp
+++ clang/lib/ASTMatchers/ASTMatchFinder.cpp
@@ -374,6 +374,12 @@
 return true;
   }
 
+  bool VisitObjCCompatibleAliasDecl(ObjCCompatibleAliasDecl *CAD) {
+const ObjCInterfaceDecl *InterfaceDecl = CAD->getClassInterface();
+CompatibleAliases[InterfaceDecl].insert(CAD);
+return true;
+  }
+
   bool TraverseDecl(Decl *DeclNode);
   bool TraverseStmt(Stmt *StmtNode, DataRecursionQueue *Queue = nullptr);
   bool TraverseType(QualType TypeNode);
@@ -433,6 +439,11 @@
   BoundNodesTreeBuilder *Builder,
   bool Directly) override;
 
+  bool objcClassIsDerivedFrom(const ObjCInterfaceDecl *Declaration,
+  const Matcher &Base,
+  BoundNodesTreeBuilder *Builder,
+   

[PATCH] D60543: [clang] Update isDerivedFrom to support Objective-C classes 🔍

[Stephane Moore via Phabricator via cfe-commits]

stephanemoore planned changes to this revision.
stephanemoore added a comment.

Thanks for the input! I will get started on making changes accordingly.




Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:2642-2649
+  if (const auto *InterfaceDecl = dyn_cast(&Node)) {
+// 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;
+}

aaron.ballman wrote:
> This should probably be done similar to how `classIsDerivedFrom()` works. For 
> instance, there's some type alias matching logic that this does not 
> replicate, but we probably want.
Upon first glance I had determined that the type aliasing logic in 
`classIsDerivedFrom()` was particular to C++. On second consideration, we will 
probably want to make sure that compatibility aliases are handled correctly for 
Objective-C. I will take a look into making sure that works as expected.


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] Update isDerivedFrom to support Objective-C classes 🔍

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:2638
+  // Check if the node is a C++ struct/union/class.
+  if (const auto *RecordDecl = dyn_cast(&Node))
+return Finder->classIsDerivedFrom(RecordDecl, Base, Builder);

I'd prefer this be named `RD` so that it doesn't conflict with the name of the 
`RecordDecl` type.



Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:2642-2649
+  if (const auto *InterfaceDecl = dyn_cast(&Node)) {
+// 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;
+}

This should probably be done similar to how `classIsDerivedFrom()` works. For 
instance, there's some type alias matching logic that this does not replicate, 
but we probably want.



Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:2667-2672
+  if (const auto *InterfaceDecl = dyn_cast(&Node)) {
+return Matcher(M).matches(*InterfaceDecl, Finder,
+ Builder);
+  }
+
+  llvm_unreachable("Not a valid polymorphic type");

How about:
```
const auto *InterfaceDecl = cast(&Node);
return Matcher...
```
We can rely on `cast<>` to assert if the node is of an unexpected type. Same 
suggestions below.


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] Update isDerivedFrom to support Objective-C classes 🔍

[Stephane Moore via Phabricator via cfe-commits]

stephanemoore accepted this revision.
stephanemoore added a comment.
This revision is now accepted and ready to land.

Okay I now have an implementation of Option 2 that //works//.

I was hoping to find a more elegant solution but since this is the first 
working implementation of Option 2 I was able to produce, I figured I would 
present it to get feedback. My lack of familiarity with polymorphic matchers 
could be causing me to overlook some potential options. If anyone can think of 
a more elegant solution, I would be happy to implement it.


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] Update isDerivedFrom to support Objective-C classes 🔍

[Stephane Moore via Phabricator via cfe-commits]

stephanemoore updated this revision to Diff 201385.
stephanemoore marked an inline comment as done.
stephanemoore added a comment.

Add missing braces to multi-line if statements.


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,38 @@
 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));
+}
+
 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,31 +2622,94 @@
 ///   typedef Foo X;
 ///   class Bar : public Foo {};  // derived from a type that X is a typedef of
 /// \endcode
-AST_MATCHER_P(CXXRecordDecl, isDerivedFrom,
-  internal::Matcher, Base) {
-  return Finder->classIsDerivedFrom(&Node, Base, Builder);
+///
+/// In the following example, Bar matches isDerivedFrom(hasName("NSObject"))
+/// \code
+///   @interface NSObject @end
+///   @interface Bar : NSObject @end
+/// \endcode
+///
+/// Usable as: Matcher, Matcher
+AST_POLYMORPHIC_MATCHER_P(
+isDerivedFrom,
+AST_POLYMORPHIC_SUPPORTED_TYPES(CXXRecordDecl, ObjCInterfaceDecl),
+internal::Matcher, Base) {
+  // Check if the node is a C++ struct/union/class.
+  if (const auto *RecordDecl = dyn_cast(&Node))
+return Finder->classIsDerivedFrom(RecordDecl, Base, Builder);
+
+  // Check if the node is an Objective-C class.
+  if (const auto *InterfaceDecl = dyn_cast(&Node)) {
+// 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_POLYMORPHIC_MATCHER_P_OVERLOAD(
+isDerivedFrom,
+AST_POLYMORPHIC_SUPPORTED_TYPES(CXXRecordDecl, ObjCInterfaceDecl),
+std::string, BaseName, 1) {
   assert(!BaseName.empty());
-  return isDerivedFrom(hasName(BaseName)).matches(Node, Finder, Builder);
+
+  const auto M = isDerivedFrom(hasName(BaseNam

[PATCH] D60543: [clang] Update isDerivedFrom to support Objective-C classes 🔍

[Stephane Moore via Phabricator via cfe-commits]

stephanemoore updated this revision to Diff 201384.
stephanemoore added a comment.

Update isDerivedFrom and related matchers to polymorphic matchers.


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,38 @@
 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));
+}
+
 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,31 +2622,91 @@
 ///   typedef Foo X;
 ///   class Bar : public Foo {};  // derived from a type that X is a typedef of
 /// \endcode
-AST_MATCHER_P(CXXRecordDecl, isDerivedFrom,
-  internal::Matcher, Base) {
-  return Finder->classIsDerivedFrom(&Node, Base, Builder);
+///
+/// In the following example, Bar matches isDerivedFrom(hasName("NSObject"))
+/// \code
+///   @interface NSObject @end
+///   @interface Bar : NSObject @end
+/// \endcode
+///
+/// Usable as: Matcher, Matcher
+AST_POLYMORPHIC_MATCHER_P(
+isDerivedFrom,
+AST_POLYMORPHIC_SUPPORTED_TYPES(CXXRecordDecl, ObjCInterfaceDecl),
+internal::Matcher, Base) {
+  // Check if the node is a C++ struct/union/class.
+  if (const auto *RecordDecl = dyn_cast(&Node))
+return Finder->classIsDerivedFrom(RecordDecl, Base, Builder);
+
+  // Check if the node is an Objective-C class.
+  if (const auto *InterfaceDecl = dyn_cast(&Node)) {
+// 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_POLYMORPHIC_MATCHER_P_OVERLOAD(
+isDerivedFrom,
+AST_POLYMORPHIC_SUPPORTED_TYPES(CXXRecordDecl, ObjCInterfaceDecl),
+std::string, BaseName, 1) {
   assert(!BaseName.empty());
-  return isDerivedFrom(hasName(BaseName)).matches(Node, Finder, Builder);
+
+  const auto M = isDerivedFrom(hasName(BaseName));
+
+  if (const auto *Rec

[PATCH] D60543: [clang] Update isDerivedFrom to support Objective-C classes 🔍

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added a comment.

> I prefer Option 2 because it is a cleaner, more understandable design for the 
> matchers.

I agree with Aaron. Clang or Clang Tooling provide no promise of source 
stability. Of course we should not break things just because we can, but API 
coherence and maintainability of Clang Tooling code are good reasons.


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] Update isDerivedFrom to support Objective-C classes 🔍

[Stephane Moore via Phabricator via cfe-commits]

stephanemoore planned changes to this revision.
stephanemoore added a comment.

> Out of curiosity, how invasive is Option 2 within our own code base?

I am still working on getting something working but I don't anticipate anything 
notably invasive.

> Does that option require fixing a lot of code?

I believe it doesn't require fixing a lot of code within our codebase. I think 
we just need to fix the overloaded `isDerivedFrom` matcher and both 
`isSameOrDerivedFrom` matchers.

> Are the breakages loud or silent?

I expect that the breakages should be loud compilation failures.

> Is the code easy to modify, or are there corner cases where this option 
> becomes problematic?

I am still working on putting together something that works. I am still getting 
familiar with polymorphic matchers so I probably can't give a reliable 
assessment of the ease of the code changes just yet. I imagine that the changes 
in this commit to convert the overloaded `isDerivedFrom` matcher and the two 
`isSameOrDerivedFrom` matchers could be demonstrative to others as to how to 
resolve breakages. I am not confident enough to state that with certainty 
though.

> I prefer Option 2 because it is a cleaner, more understandable design for the 
> matchers. If it turns out that this option causes a hard break (rather than 
> silently changing matcher behavior) and the changes needed to get old code to 
> compile again are minimal, I think it may be a reasonable choice.

I agree that Option 2 is preferable if we are allowed to require a non-zero 
amount of migration work from downstream clients. I will continue working on 
Option 2 and will return once I have something working.


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] Update isDerivedFrom to support Objective-C classes 🔍

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added reviewers: klimek, alexfh.
aaron.ballman added a comment.

Adding some more AST matcher reviewers to see if there are other options that 
we've not considered.


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] Update isDerivedFrom to support Objective-C classes 🔍

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D60543#1497576 , @stephanemoore 
wrote:

> I did some digging and I believe there are two approaches that we can take to 
> extend `isDerivedFrom` to support Objective-C classes.
>
> **Option 1: Match on Common Ancestor Declaration Type**:
>  Convert `isDerivedFrom` to match on the common ancestor declaration type, 
> `NamedDecl`, and return `false` for
>  declaration types other than `CXXRecordDecl` and `ObjCInterfaceDecl`.
>
> Advantages:
>  • Largely works in-place without requiring major changes to matchers built 
> on top of `isDerivedFrom`.
>
> Disadvantages:
>  • Allows nonsensical matchers, e.g., `functionDecl(isDerivedFrom("X"))`.
>
> **Option 2: Convert to Polymorphic Matcher**:
>  Convert `isDerivedFrom` to a polymorphic matcher supporting `CXXRecordDecl` 
> and `ObjCInterfaceDecl`.
>
> Advantages:
>  • Restricts usage of `isDerivedFrom` to `CXXRecordDecl` and 
> `ObjCInterfaceDecl`.
>
> Disadvantages:
>  • Potentially requires many or all matchers using `isDerivedFrom` to 
> refactor to adapt to the polymorphic matcher interface.
>
> **Evaluation**
>  I have been going back and forth as to which approach is superior. Option 1 
> promotes source stability at the cost of usability while
>  option 2 seems to promote usability at the cost of source stability. I 
> exported a portrayal of option 1 for consideration as it
>  required less invasive changes. I can see arguments supporting both 
> approaches.
>
> What do you think? Which of the two approaches do you think we should go 
> with? Is there another approach that I have not thought of?


This is a great breakdown, thank you for providing it!

Out of curiosity, how invasive is Option 2 within our own code base? Does that 
option require fixing a lot of code? Are the breakages loud or silent? Is the 
code easy to modify, or are there corner cases where this option becomes 
problematic? I prefer Option 2 because it is a cleaner, more understandable 
design for the matchers. If it turns out that this option causes a hard break 
(rather than silently changing matcher behavior) and the changes needed to get 
old code to compile again are minimal, I think it may be a reasonable choice.


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] Update isDerivedFrom to support Objective-C classes 🔍

[Stephane Moore via Phabricator via cfe-commits]

stephanemoore added inline comments.



Comment at: clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp:566
+  notMatchesObjC("@interface Y @end typedef Y X; @interface Z : X @end",
+  ZIsDerivedFromX));
+  EXPECT_TRUE(

(note that there are unresolved formatting issues that I intend to fix after I 
establish which approach to pursue)


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] Update isDerivedFrom to support Objective-C classes 🔍

[Stephane Moore via Phabricator via cfe-commits]

stephanemoore added a comment.

I did some digging and I believe there are two approaches that we can take to 
extend `isDerivedFrom` to support Objective-C classes.

**Option 1: Match on Common Ancestor Declaration Type**:
Convert `isDerivedFrom` to match on the common ancestor declaration type, 
`NamedDecl`, and return `false` for
declaration types other than `CXXRecordDecl` and `ObjCInterfaceDecl`.

Advantages:
• Largely works in-place without requiring major changes to matchers built on 
top of `isDerivedFrom`.

Disadvantages:
• Allows nonsensical matchers, e.g., `functionDecl(isDerivedFrom("X"))`.

**Option 2: Convert to Polymorphic Matcher**:
Convert `isDerivedFrom` to a polymorphic matcher supporting `CXXRecordDecl` and 
`ObjCInterfaceDecl`.

Advantages:
• Restricts usage of `isDerivedFrom` to `CXXRecordDecl` and `ObjCInterfaceDecl`.

Disadvantages:
• Potentially requires many or all matchers using `isDerivedFrom` to refactor 
to adapt to the polymorphic matcher interface.

**Evaluation**
I have been going back and forth as to which approach is superior. Option 1 
promotes source stability at the cost of usability while
option 2 seems to promote usability at the cost of source stability. I exported 
a portrayal of option 1 for consideration as it
required less invasive changes. I can see arguments supporting both approaches.

What do you think? Which of the two approaches do you think we should go with? 
Is there another approach that I have not thought of?


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