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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
