shuaiwang marked 5 inline comments as done.
shuaiwang added a comment.
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.
================
Comment at: clang-tidy/utils/ExprMutationAnalyzer.cpp:88
+
+const Stmt *ExprMutationAnalyzer::findDirectMutation(const Expr *Exp) {
+ // LHS of any assignment operators.
----------------
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`)
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
