[PATCH] D60543: [clang] Add matcher for subclasses of Objective-C interfaces 🔍

Wed Apr 17 11:28:03 PDT 2019

aaron.ballman added inline comments.

================
Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:1479
+/// \endcode
+AST_MATCHER_P(ObjCInterfaceDecl, isSubclassOfInterface,
+              internal::Matcher<ObjCInterfaceDecl>,
----------------
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