[PATCH] D45679: [clang-tidy] Add ExprMutationAnalyzer, that analyzes whether an expression is mutated within a statement.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In https://reviews.llvm.org/D45679#1088696, @shuaiwang wrote:

> I've reverted the handling of DeclRefExpr to volatile after rethinking about it.
>
> The purpose of this analyzer is to find whether the given Expr is mutated within a scope, but doesn't really care about external changes. In other words we're trying to find mutations explicitly written within the specified scope.


Okay, that seems like a good reason to ignore volatile qualifiers. Thank you for the explanation!



================
Comment at: clang-tidy/utils/ExprMutationAnalyzer.cpp:88
+
+const Stmt *ExprMutationAnalyzer::findDirectMutation(const Expr *Exp) {
+  // LHS of any assignment operators.
----------------
shuaiwang wrote:
> aaron.ballman wrote:
> > aaron.ballman wrote:
> > > JonasToth wrote:
> > > > shuaiwang wrote:
> > > > > 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.
> > > > May I ask the exact semantics for `volatile`? I would add the test to my check, too.
> > > > 
> > > > It is possible to write `const volatile int m = 42;`, but if i understand correctly `m` could still change by hardware, or would this be UB?
> > > > If the `const` is missing, one must always assume external modification, right?
> > > > 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.
> > > 
> > > What I am getting at is that the code in `findDirectMutation()` seems to go through a lot of effort looking for expressions that have side effects and I was wondering if we could leverage that call instead of duplicating the effort.
> > > May I ask the exact semantics for volatile? I would add the test to my check, too.
> > 
> > Basically, volatile objects can change in ways unknown to the implementation, so a volatile read must always perform an actual read (because the object's value may have changed since the last read) and a volatile write must always be performed as well (because writing a value may have further side effects if the object is backed by some piece of interesting hardware).
> > 
> > > It is possible to write const volatile int m = 42;, but if i understand correctly m could still change by hardware, or would this be UB?
> > 
> > That could still be changed by hardware, and it would not be UB. For instance, the variable might be referencing some hardware sensor and so you can only read the values in (hence it's const) but the values may be changed out from under you (hence, it's not constant).
> > 
> > > If the const is missing, one must always assume external modification, right?
> > 
> > You have to assume external modification regardless.
> Unfortunately I don't think HasSideEffects help in here. We're finding one particular side effect that is mutating the specified Expr so we need to have explicitly `equalsNode(Exp)` for all the matches. `HasSideEffects` can simply claim `f(a, b)` or even `g()` has side effects (given `void f(Foo&, const Bar&)`, `void g()`) while we need to require a link to the specific Expr we're analyzing (i.e. `f(a, b)` is mutating `a` but not `b`)
Ah, okay, thank you for the explanation.

I think you may want to look at the implementation for `HasSideEffect()` to identify other cases you may want to support. For instance, adding tests for statement expressions, paren expressions, bizarre things like VLA mutations in otherwise unevaluated contexts, etc.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D45679