[PATCH] D18265: [clang-tidy] New: checker misc-assign-operator-return

[Samuel Benzaquen via cfe-commits]

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