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


================
Comment at: clang-tidy/utils/ExprMutationAnalyzer.cpp:88
+
+const Stmt *ExprMutationAnalyzer::findDirectMutation(const Expr *Exp) {
+  // LHS of any assignment operators.
----------------
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.


================
Comment at: clang-tidy/utils/ExprMutationAnalyzer.cpp:88
+
+const Stmt *ExprMutationAnalyzer::findDirectMutation(const Expr *Exp) {
+  // LHS of any assignment operators.
----------------
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.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D45679