[cfe-commits] [PATCH] Implements hasAncestor.

[Manuel Klimek]

================
Comment at: include/clang/ASTMatchers/ASTMatchersInternal.h:461
@@ +460,3 @@
+                            llvm::is_base_of<Stmt, T>::value),
+                           only_Decl_or_Stmt_allowed_for_recursive_matching);
+    return matchesAncestorOf(ast_type_traits::DynTypedNode::create(Node),
----------------
Michael Diamond wrote:
> Daniel Jasper wrote:
> > Michael Diamond wrote:
> > > Daniel Jasper wrote:
> > > > Michael Diamond wrote:
> > > > > Manuel Klimek wrote:
> > > > > > Daniel Jasper wrote:
> > > > > > > Unlike descendants, all node types should have ancestors. Is it intentional to not allow this for QualTypes or possibly other types? 
> > > > > > > - If there is a good reason for it: Comment.
> > > > > > > - If it would be significant additional work: Command + FIXME
> > > > > > > - Otherwise: Implement ;-)
> > > > > > Like with the other methods here, there are many more nodes for which we want them implemented than are currently implemented.
> > > > > > 
> > > > > > You're right that for hasAncestor probably pretty much all nodes make sense. Added a class-level comment that outlines the differences.
> > > > > I really need this to work for QualTypes. Any chance of getting that in this CL? We don't have to update both ancestor and child matchers together necessarily.
> > > > Could you quickly elaborate on what you are in need of matching? This is actually not quite as easy (from a design perspective) and I think it should be in a different patch. But maybe we can find a way around it :-).
> > > If it has to hold of to another patch, alright, but I need it ASAP. :)
> > > 
> > > Basically, I am refactoring from using one class to using a parent of it. However, for references and pointers, I want to use an interface that is a parent of that class.
> > > 
> > > Example:
> > > Interface
> > >   |
> > > Parent
> > >   |
> > > CurrentClass
> > > 
> > > I want "CurrentClass*" to become "Interface*", but "new CurrentClass" to become "new Parent".
> > Interesting. I don't really see why you would need hasAncestor for QualTypes to solve this. More precisely, I don't see how QualType matching is going to help at all. The problem with QualTypes (what I also meant above) is that they don't have a natural identity and we don't yet know what to do about that with hasAncestor. More importantly, QualTypes don't have a SourceLocation you could use for refactoring.
> > 
> > I think what would help a lot would be TypeLoc-Matchers, but maybe that is not necessary. Some thoughts:
> > 
> >   DeclarationMatcher current = recordDecl(hasName("CurrentClass"));
> >   TypeMatcher pointerOrReference =
> >     anyOf(pointsTo(current), references(current));
> >   MatchFinder->addMatcher(decl(anyOf(
> >     declaratorDecl(hasType(pointerOrReference)).bind("decl"),
> >     functionDecl(returns(pointerOrReference)).bind("decl"))), Callback);
> > 
> > If decl is a VarDecl or FieldDecl, you can get the SourceLocation of the type out of the result of getTypeSourceInfo(). For FunctionDecls, you have to look at the result and I am not entirely sure how to get to the TypeSourceInfo of that. Also, pointerOrReference needs to be slightly extended to handle arrays and you can do something similar for non-pointer-or-reference cases.
> > 
> > Then you will need to match template parameters.. That should be fairly straight forward.
> > 
> > Then there are CXXConstructExprs (new CurrentClass()).. I think they don't involve QualTypes or TypeLocs. You basically have to
> > 
> >   addMatcher(constructExpr(hasDeclaration(
> >     constructorDecl(ofClass(current)))).bind("constructExpr"), Callback);
> > 
> > .. and then hope that CXXConstructExpr::getLocation() returns the Token that you need to replace :-/.
> > 
> > And then you might need to worry about static members of the class being accessed (no QualTypes / TypeLocs involved).
> > 
> > Ah, and using declarations ..
> > 
> > So lots of stuff to do, I hope I have not rambled too much. But I don't see imminent need for hasAncestor() on QualTypes. hasDescendant() might actually help more with the pointerOrReferenceOrArrayOr..-business. But of course, I might have overlooked a really easy thing to do with hasAncestor().
> > 
> Yes, it looks like I do want TypeLoc-Matchers. I've tried putting together a series of complicated matchers like you describe, but there really are a lot of cases. It got to the point where I'd rather just use regexes.
> 
> I guess what I want is something like (yes, some of these matchers are probably off, but this is the idea):
> 
>   addMatcher(typeLocMatcher(hasType(hasAnyQualifiers(),
>       pointerOrReferenceTo(class(hasName("CurrentClass"))))), Callback);
>   addMatcher(typeLocMatcher(hasType(hasAnyQualifiers(),
>       class(hasName("CurrentClass")),
>       not(hasAncestor(isPointerOrReference()))), OtherCallback);
Let me take a quick step back :)

We think that by giving you TypeLoc and NestedNameSpecifierLoc visitors we'll cover all of the use cases you need (the constructor calls will need to be special-handled, but you'll want to do that anyway).

Now, the question is whether you want to:
a) contribute to getting there
b) wait for us getting there

We're of course thrilled if you're willing to contribute, but I'm strongly voting against shooting from the hip here - there will be design arguments (as you have noticed by now) and we'll need to think the stuff through.

As for your question with the current CL - I was on purpose not implementing type recursion, as I wanted to talk with you about what the state of your type matching patch is / where we want to go design-wise first, and do that in a subsequent CL.

That said, I think we'll need some time to figure all this out - if you can satisfy your current use case with regexps and a few manual fix-ups afterwards, that might be a better solution for you right now.


http://llvm-reviews.chandlerc.com/D36