[PATCH] D45679: [clang-tidy] Add a helper function isModified, that checks whether an expression is modified within a statement.

[Shuai Wang via Phabricator via cfe-commits]

shuaiwang added inline comments.


================
Comment at: clang-tidy/utils/ExprMutationAnalyzer.cpp:82-83
+    const auto *E = RefNodes.getNodeAs<Expr>("expr");
+    if (findMutation(E))
+      return E;
+  }
----------------
aaron.ballman wrote:
> Why does this not return the result of `findMutation()` like the other call above?
find*Mutation is intended to return where the given Expr is mutated.
To make it a bit more useful, for Decl's I'm returning the DeclRefExpr and caller can chose to follow it to find where the DeclRefExpr is mutated if needed.
As a concrete example:
```
struct A { int x; };
struct B { A a; };
struct C { B b; };
C c;
C& c2 = c;
c2.b.a.x = 10;
```
If we start with DeclRefExpr to `c` we'll find mutation at a DeclRefExpr to `c2`, following that we can find the mutation Stmt being `c2.b.a.x = 10`.
Returning `E` here makes it possible to reveal intermediate checkpoint instead of directly saying `c` is mutated at `c2.b.a.x = 10` which might be confusing.


================
Comment at: clang-tidy/utils/ExprMutationAnalyzer.cpp:88
+
+const Stmt *ExprMutationAnalyzer::findDirectMutation(const Expr *Exp) {
+  // LHS of any assignment operators.
----------------
aaron.ballman wrote:
> Should this also consider a DeclRefExpr to a volatile-qualified variable as a direct mutation?
> 
> What about using `Expr::HasSideEffect()`?
Good catch about DeclRefExpr to volatile.

`HasSideEffects` means something different. Here find*Mutation means find a Stmt **in ancestors** that mutates the given Expr. `HasSideEffects` IIUC means whether anything is mutated by the Expr but doesn't care about what exactly is mutated.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D45679