<br><div class="gmail_extra"><br><br><div class="gmail_quote">On Fri, Sep 7, 2012 at 10:18 PM, Manuel Klimek <span dir="ltr"><<a href="mailto:klimek@google.com" target="_blank" class="cremed">klimek@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">On Fri, Sep 7, 2012 at 9:55 PM, Chandler Carruth <<a href="mailto:chandlerc@google.com" class="cremed">chandlerc@google.com</a>> wrote:<br>
> On Fri, Sep 7, 2012 at 8:06 AM, Daniel Jasper<br>
> <<a href="mailto:reviews@llvm-reviews.chandlerc.com" class="cremed">reviews@llvm-reviews.chandlerc.com</a>> wrote:<br>
>><br>
>> Hi klimek,<br>
>><br>
>> Change the behavior of the isDerivedFrom-matcher to not match on the class<br>
>> itself. This caused some confusion (intuitively, a class is not derived from<br>
>> itself) and makes it hard to write certain matchers, e.g. "match and bind<br>
>> any pair of base and subclass".<br>
>><br>
>> The original behavior can be achieved with a new isA-matcher. Similar to<br>
>> all other matchers, this matcher has the same behavior and name as the<br>
>> corresponding AST-entity - in this case the isa<>() function.<br>
><br>
><br>
> This is not a new idea. In fact, when we first built the isDerivedFrom<br>
> matcher over a year ago Manuel and others argued for what you propose. I<br>
> argued firmly against it, and my feelings have not changed. Manuel should<br>
> have remembered that.<br>
<br>
</div>I have remembered that; I also think the situation has changed - the<br>
argument that isA would conflict with other names seems not to hold in<br>
the light of the matchers reflecting functions that you would use with<br>
other matchers - there is a duplication of domain naming inherent in<br>
what the matchers are nowadays.<br></blockquote><div><br></div><div>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?</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">
> See Sebastien Redl's comment for why I firmly believe that the current<br>
> behavior is the correct behavior: std::is_base_of behaves the exact same<br>
> way.<br></div></blockquote><div><br></div><div>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.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">
> I don't see why "math and bind any pair of base and subclass" is that hard<br>
> to write. It only requires a not matcher that tests for type equality. Can<br>
> you give an example that is hard to write, and maybe there is a better way<br>
> to write it?<br>
<br>
</div>You cannot test for type equality unless you know the type of what<br>
you're looking for.<br></blockquote><div><br></div><div>To give more concrete examples. I would like to match any parent->direct base relationship. With the new matcher, I could do:</div><div><br></div><div> recordDecl(isDerivedFrom(recordDecl.bind("base"))).bind("derived")</div>
<div><br></div><div>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.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im"><br>
> I also strongly object to the name "isA" as a matcher. We already have an<br>
> "isa<..>()" utility within LLVM and Clang that is extremely widely used. I<br>
> *do not want* a case-difference to select between the two in any<br>
> circumstance. And again, I raised this objection when the idea was first<br>
> proposed over a year ago.<br>
<br>
</div>Why's that different from having all the other name collisions?<br>
<div class="im"><br>
> So, let's discuss the actual problems with writing matchers, and see if we<br>
> can find solutions for them?<br>
<br>
</div>I strongly vote for re-opening this box. I have seen enough users<br>
being surprised.<br></blockquote><div><br></div><div>Me, too obviously. Two important points:</div><div>1. You can express more.<br></div><div><div>2. It is confusing.</div></div><div><br></div><div>#1 just means we need the two matchers, and I think nobody is against that.</div>
<div>#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.</div><div><br></div><div>Cheers,</div><div>Daniel</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Cheers,<br>
/Manuel<br>
<div class=""><div class="h5"><br>
><br>
> -Chandler<br>
><br>
> PS: Just to be clear, I claim that the AST matchers are currently in the<br>
> exact same situation as any other C++ interfaces in Clang. While we should<br>
> not gratuitously break out-of-tree code, we should continue to feel free to<br>
> change the API in backwards incompatible ways, and document these changes to<br>
> help people with out-of-tree code update to match. So, I'm not worried about<br>
> that aspect of this patch. =]<br>
</div></div></blockquote></div><br></div>