[PATCH] D27187: [clang-tidy] Do not move parameter if only DeclRefExpr occurs inside of a loop

Alexander Kornienko via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Tue Dec 13 08:43:18 PST 2016

alexfh accepted this revision.
alexfh added a comment.
This revision is now accepted and ready to land.

LG modulo comment.

Comment at: clang-tidy/utils/DeclRefExprUtils.cpp:127
+      match(findAll(declRefExpr(equalsNode(&DeclRef),
+                                unless(hasAncestor(stmt(anyOf(
+                                    forStmt(), cxxForRangeStmt(), whileStmt(),
flx wrote:
> alexfh wrote:
> > flx wrote:
> > > alexfh wrote:
> > > > How will this work with lambdas / local classes declared inside a loop? Not sure if this case is going to happen in real code, but we'd better be clear about the limitations of the implementation.
> > > Why would this not work? Could you give an example? The way the function is written it handles my the use case for identifying when moving the parameter is not safe, so I could also just move it into the UnnecessaryValueParamCheck.
> > I was thinking about a case where a loop this matcher finds is outside of the function definition and shouldn't be considered:
> > 
> >   void F() {
> >     for (;;) {
> >       struct C {
> >         void f(ExpensiveMovableType E) {
> >           auto F = E;
> >         }
> >       };
> >     }
> >   }
> > 
> This case is not an issue in the check as we're passing f's body statement to the hasLoopStmtAncestor function, so the search is scoped to f's body.
> If you're concerned about the case where someone calls this with  F's body but expects it to be scoped to f I can just move this function into the check and make it an implementation detail.
Making it an implementation detail until other potential users appear sounds good.


More information about the cfe-commits mailing list