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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
