[PATCH] D60543: [clang] Add matcher for subclasses of Objective-C interfaces 🔍
Aaron Ballman via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Wed Apr 17 11:16:26 PDT 2019
aaron.ballman added inline comments.
Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:1479
> 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?
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
More information about the cfe-commits