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

[Daniel Jasper]

On Fri, Sep 7, 2012 at 10:18 PM, Manuel Klimek <klimek at google.com> wrote:

> 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.
>

Additionally, it is not easy to confuse the two, I think. First of, isA
lives in clang::ast_matchers. So you would either have to work in that
namespace or explicitly import it with using directives.  Why is the
similarity in name (with one letter being different in case) desirable for
all other AST matchers, but not for isA? Is there an inherent difference
between isA <-> isa and expr <-> Expr?


> > 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.
>

This I did not know. I still don't think isDerivedFrom matching the class
itself is intuitive at all, but this sheds some doubt on it. I firmly
believe that neither behavior can be called "correct" as there is no
definition of what correct is. There is std::is_base_of, but there is also
what the standard says (e.g. "A class B is a base class of a class D if it
is a direct base class of D or a direct base class of one of D’s base
classes") and what people (not essentially working on a C++ compiler)
intuitively think.


> > 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.
>

To give more concrete examples. I would like to match any parent->direct
base relationship. With the new matcher, I could do:

  recordDecl(isDerivedFrom(recordDecl.bind("base"))).bind("derived")

With the old behavior, I can't implement this as part of the matcher
language. And I think there is a more general case here. Basically, you
can't match base classes if the derived class might also have the matched
property. E.g. match all classes derived from a class with a constructor
having a specific argument.


> > 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.
>

Me, too obviously. Two important points:
1. You can express more.
2. It is confusing.

#1 just means we need the two matchers, and I think nobody is against that.
#2 basically concerns the names of the matchers. I am open to discussion,
but I quite strongly feel that this is the most intuitive interface.

Cheers,
Daniel


> 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. =]
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120908/dd881fe0/attachment.html>