[cfe-commits] [PATCH] Implements hasAncestor.
Manuel Klimek
reviews at llvm-reviews.chandlerc.com
Fri Sep 7 01:20:27 PDT 2012
================
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
More information about the cfe-commits
mailing list