[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 <https://reviews.llvm.org/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