[PATCH] D58894: [analyzer] Handle modification of vars inside an expr with comma operator

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

Nice, i like this!
I think the test coverage is good.
But what about other `equalsNode()` that you didn't change?
Do some of them need this change too?



================
Comment at: lib/Analysis/ExprMutationAnalyzer.cpp:27
 
+AST_MATCHER_P(Expr, skipCommaOps,
+             ast_matchers::internal::Matcher<Expr>, InnerMatcher) {
----------------
(Can anyone suggest a better name maybe?
I'm not sure this is the most correct name, but i can't suggest a better alternative.)


================
Comment at: lib/Analysis/ExprMutationAnalyzer.cpp:223
                  cxxOperatorCallExpr(callee(NonConstMethod),
                                      hasArgument(0, equalsNode(Exp))),
                  callExpr(callee(expr(anyOf(
----------------
Not changed?


================
Comment at: lib/Analysis/ExprMutationAnalyzer.cpp:225-227
                      unresolvedMemberExpr(hasObjectExpression(equalsNode(Exp))),
                      cxxDependentScopeMemberExpr(
                          hasObjectExpression(equalsNode(Exp)))))))));
----------------
Can these two happen with comma op?


================
Comment at: lib/Analysis/ExprMutationAnalyzer.cpp:240
       castExpr(hasCastKind(CK_ArrayToPointerDecay),
                unless(hasParent(arraySubscriptExpr())), has(equalsNode(Exp)));
   // Treat calling `operator->()` of move-only classes as taking address.
----------------
Same, can this happen with comma op?


================
Comment at: lib/Analysis/ExprMutationAnalyzer.cpp:248
                                                returns(nonConstPointerType()))),
                           argumentCountIs(1), hasArgument(0, equalsNode(Exp)));
 
----------------
And here too


================
Comment at: lib/Analysis/ExprMutationAnalyzer.cpp:265
                                  hasType(templateTypeParmType())))),
                hasAnyArgument(equalsNode(Exp))),
       cxxUnresolvedConstructExpr(hasAnyArgument(equalsNode(Exp))));
----------------
And here


================
Comment at: lib/Analysis/ExprMutationAnalyzer.cpp:279
   // adding const.)
   const auto AsNonConstRefReturn = returnStmt(hasReturnValue(equalsNode(Exp)));
 
----------------
And here


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58894/new/

https://reviews.llvm.org/D58894