sbenza added inline comments.
================
Comment at: clang-tidy/misc/AssignOperatorCheck.cpp:63
@@ +62,3 @@
+
+ Finder->addMatcher(returnStmt(IsBadReturnStatement,
hasAncestor(IsGoodAssign))
+ .bind("returnStmt"),
----------------
baloghadamsoftware wrote:
> sbenza wrote:
> > baloghadamsoftware wrote:
> > > sbenza wrote:
> > > > baloghadamsoftware wrote:
> > > > > sbenza wrote:
> > > > > > I dislike these uses of hasAnscestor. They are kind of slow.
> > > > > > But more importantly, they break with nested functions/types.
> > > > > > This particular example is not checking that the return statement
> > > > > > is from the assignment operator, only that it is within it. For
> > > > > > example, it would match a lambda.
> > > > > > I think this would trip the check:
> > > > > >
> > > > > > F& operator=(const F& o) {
> > > > > > std::copy_if(o.begin(), o.end(), begin(), [](V v) { return v
> > > > > > > 0; });
> > > > > > return *this;
> > > > > > }
> > > > > I can change it to hasDescendant if it is faster, but it does not
> > > > > solve the lambda problem. No solution for that comes to my mind with
> > > > > the existing matchers. Maybe a new matcher hasDescendantStatement
> > > > > could help which only traverses statements down the AST. Is this the
> > > > > right way to go?
> > > > hasDescendant has the same problem has hasAnscestor.
> > > > I think the best is to write a matcher like:
> > > >
> > > > AST_MATCHER_P(ReturnStmt, forFunction,
> > > > internal::Matcher<FunctionDecl>, InnerMatcher) {
> > > > ...
> > > > }
> > > >
> > > > In there we can find the right FunctionDecl that encloses the return
> > > > statement and apply the matcher.
> > > > This matcher seems like a good candidate to add to ASTMatchers.h
> > > Maybe I am wrong, but your proposal also seems a bottom-up matcher which
> > > is slow. That is the reason I proposed a top-down matcher, e.g.
> > > hasDescendantStatement or something like this which is top-down and only
> > > traverses statements so it does not search in a lambda which is a
> > > declaration.
> > hasAnscestor is slow because it is way too general. There are tons of
> > virtual function calls, cache lookups, memory allocations, etc. And the
> > search will not stop until it find a match or runs out of anscestors. This
> > means it will go all the way to the translationUnitDecl before it figures
> > out that there are no matches.
> > Every parent will be run through the matcher.
> >
> > What I propose is a simple matcher that:
> > 1. Finds the enclosing FunctionDecl for a statement.
> > 2. Runs the matcher on it, if there is an enclosing function.
> >
> > (1) can be done without any virtual function call and with no/minimal
> > memory allocations.
> > (2) will be done only once on the found node.
> >
> I did something like your proposal and it is working, but before uploading a
> patch I have a question. There are some nodes, even statement having multiple
> parents, probably due to template instantiations. Is it enough if we chose
> the first parent here (thus going up only to the template itself) or should
> we do a depth-first (better to say height-search here, since we are not
> searching downwards in a tree but upwards in a DAG) search here and go up to
> every parent? hasAncestor uses breadth-first search which is out of the
> question here.
Yes, with templates you can have multiple parents.
I think you can use any of the parents, but it would be unspecified which
FunctionDecl you get.
Maybe step (1) can return a list of FunctionDecls instead.
In this particular case you might need the particular template instantiation
because the return type might be a dependent type.
http://reviews.llvm.org/D18265
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
