[cfe-commits] [PATCH] Change counter-intuitive behavior of isDerivedFrom

[Manuel Klimek]

On Fri, Sep 7, 2012 at 9:55 PM, Chandler Carruth <chandlerc at google.com> wrote:
> On Fri, Sep 7, 2012 at 8:06 AM, Daniel Jasper
> <reviews at llvm-reviews.chandlerc.com> wrote:
>>
>> Hi klimek,
>>
>> Change the behavior of the isDerivedFrom-matcher to not match on the class
>> itself. This caused some confusion (intuitively, a class is not derived from
>> itself) and makes it hard to write certain matchers, e.g. "match and bind
>> any pair of base and subclass".
>>
>> The original behavior can be achieved with a new isA-matcher.  Similar to
>> all other matchers, this matcher has the same behavior and name as the
>> corresponding AST-entity - in this case the isa<>() function.
>
>
> This is not a new idea. In fact, when we first built the isDerivedFrom
> matcher over a year ago Manuel and others argued for what you propose. I
> argued firmly against it, and my feelings have not changed. Manuel should
> have remembered that.

I have remembered that; I also think the situation has changed - the
argument that isA would conflict with other names seems not to hold in
the light of the matchers reflecting functions that you would use with
other matchers - there is a duplication of domain naming inherent in
what the matchers are nowadays.

> See Sebastien Redl's comment for why I firmly believe that the current
> behavior is the correct behavior: std::is_base_of behaves the exact same
> way.
>
> I don't see why "math and bind any pair of base and subclass" is that hard
> to write. It only requires a not matcher that tests for type equality. Can
> you give an example that is hard to write, and maybe there is a better way
> to write it?

You cannot test for type equality unless you know the type of what
you're looking for.

> I also strongly object to the name "isA" as a matcher. We already have an
> "isa<..>()" utility within LLVM and Clang that is extremely widely used. I
> *do not want* a case-difference to select between the two in any
> circumstance. And again, I raised this objection when the idea was first
> proposed over a year ago.

Why's that different from having all the other name collisions?

> So, let's discuss the actual problems with writing matchers, and see if we
> can find solutions for them?

I strongly vote for re-opening this box. I have seen enough users
being surprised.

Cheers,
/Manuel

>
> -Chandler
>
> PS: Just to be clear, I claim that the AST matchers are currently in the
> exact same situation as any other C++ interfaces in Clang. While we should
> not gratuitously break out-of-tree code, we should continue to feel free to
> change the API in backwards incompatible ways, and document these changes to
> help people with out-of-tree code update to match. So, I'm not worried about
> that aspect of this patch. =]