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

In D58894#1416691 <https://reviews.llvm.org/D58894#1416691>, @djtodoro wrote:

> @lebedev.ri Thanks for your comment!
>
> > Is there any way to model this more generically?
> > I.e don't duplicate every naive matcher (ignoring possible presence of , op) with an variant that does not ignore ,.
> > E.g. will this handle (a,b)+=1 ?
>
> Hmm, I am not sure if there is a place for that, since this is very specific to comma operators. Any suggestions ?
>  It handles the situation (`(a,b)+=1`).
>
> > What about (a,b).callNonConstMethod(), (a,b).callConstMethod() ?
>
> This needs to be extended to support function/method calls. I guess `ExprMutationAnalyzer::findFunctionArgMutation` requires an improvement.


Well, given

  // LHS of any assignment operators.
  const auto AsAssignmentLhs =
      binaryOperator(isAssignmentOperator(), hasLHS(equalsNode(Exp)));

The problem is that `hasLHS()` may get `,` op.
In `isAssigmentWithCommma()`, you check if that is `,` op, and if so, return it's right hand.
And this duplication is the problem as far as i can see.

Now, i don't know the final solution, but have you considered adding something like:

  AST_MATCHER_P(Expr, skipCommaOps,
                ast_matchers::internal::Matcher<Expr>, InnerMatcher) {
    const Expr* Result = Node;
    while (const auto *BOComma =
                 dyn_cast_or_null<BinaryOperator>(Result->IgnoreParens())) {
      if (!BOComma->isCommaOp())
        break;
      Result = BOComma->getRHS();
    }
    return Result;
  }

and then simply changing

  // LHS of any assignment operators.
  const auto AsAssignmentLhs =
      binaryOperator(isAssignmentOperator(), hasLHS(equalsNode(Exp)));

to

  // LHS of any assignment operators.
  const auto AsAssignmentLhs =
      binaryOperator(isAssignmentOperator(), hasLHS(skipCommaOps(equalsNode(Exp))));

?


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

https://reviews.llvm.org/D58894