[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:
> > > > 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.



http://reviews.llvm.org/D18265