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

This revision was automatically updated to reflect the committed changes. Closed by commit rL355605: [analyzer] handle modification of vars inside an expr with comma operator (authored by petarj, committed by ). Herald added a project: LLVM. Herald added a subscriber: llvm-commits. Changed prior

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

lebedev.ri accepted this revision. lebedev.ri added a comment. Still LG. I'm sure you have verified that there is test coverage for all the newly-supported cases. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58894/new/ https://reviews.llvm.org/D58894

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

djtodoro updated this revision to Diff 189661. djtodoro added a comment. -add support for the rest of direct mutation cases CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58894/new/ https://reviews.llvm.org/D58894 Files: include/clang/AST/Expr.h

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

djtodoro marked an inline comment as done. djtodoro added a comment. > Anyways, this looks good in this state. > Thank you for working on this! Thanks! > If you don't intend to immediately work on fixing the rest of , cases here, > *please* do file a bug so it won't get lost. (and link it here)

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

lebedev.ri accepted this revision. lebedev.ri added a comment. This revision is now accepted and ready to land. Anyways, this looks good in this state. Thank you for working on this! If you don't intend to immediately work on fixing the rest of `,` cases here, *please* do file a bug so it won't

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

djtodoro marked an inline comment as done. djtodoro added inline comments. Comment at: lib/Analysis/ExprMutationAnalyzer.cpp:27 +AST_MATCHER_P(Expr, skipCommaOps, + ast_matchers::internal::Matcher, InnerMatcher) { lebedev.ri wrote: > djtodoro

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

lebedev.ri added inline comments. Comment at: lib/Analysis/ExprMutationAnalyzer.cpp:27 +AST_MATCHER_P(Expr, skipCommaOps, + ast_matchers::internal::Matcher, InnerMatcher) { djtodoro wrote: > lebedev.ri wrote: > > (Can anyone suggest a better name

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

djtodoro marked an inline comment as done. djtodoro added a comment. > Nice, i like this! > I think the test coverage is good. @lebedev.ri Thanks! > But what about other equalsNode() that you didn't change? > Do some of them need this change too? For sure some of them (maybe all of them) needs

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

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, +

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

djtodoro added a comment. >> @lebedev.ri I agree, thank you! I needed to be more precise in my previous >> reply, sorry for that. I thought it will be (somehow) overhead if I change >> existing, very basic, matchers. > > I indeed don't think the existing matchers should be changed to ignore

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

djtodoro updated this revision to Diff 189461. djtodoro added a comment. -add AST matcher that skips operands in comma expression -add unit tests for extended support CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58894/new/ https://reviews.llvm.org/D58894 Files:

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

lebedev.ri added a comment. In D58894#1418251 , @djtodoro wrote: > @lebedev.ri I agree, thank you! I needed to be more precise in my previous > reply, sorry for that. I thought it will be (somehow) overhead if I change > existing, very basic, matchers.

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

djtodoro added a comment. @lebedev.ri I agree, thank you! I needed to be more precise in my previous reply, sorry for that. I thought it will be (somehow) overhead if I change existing, very basic, matchers. I already implemented a static function that skips comma operands, and extended this

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

lebedev.ri added a comment. In 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

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

djtodoro added a comment. @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

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

lebedev.ri added a comment. Thank you for working on this! 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` ? What about

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

djtodoro created this revision. djtodoro added reviewers: shuaiwang, lebedev.ri. Herald added subscribers: Charusso, jdoerfert, dkrupp, donat.nagy, Szelethus, mikhail.ramalho, a.sidorin, szepet, baloghadamsoftware, xazax.hun. We should track mutation of a variable within a comma operator