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