[PATCH] Warning on unevaluated expression contexts with side effects

[Aaron Ballman]

On Wed, Dec 17, 2014 at 4:28 PM, Richard Smith <richard at metafoo.co.uk> wrote:
> On Wed, Dec 17, 2014 at 1:09 PM, Aaron Ballman <aaron at aaronballman.com>
> wrote:
>>
>> On Wed, Dec 17, 2014 at 2:49 PM, Richard Smith <richard at metafoo.co.uk>
>> wrote:
>> > @@ -3009,7 +3026,7 @@
>> >      const CastExpr *CE = cast<CastExpr>(this);
>> >      if (CE->getCastKind() == CK_LValueToRValue &&
>> >          CE->getSubExpr()->getType().isVolatileQualified())
>> > -      return true;
>> > +      return IncludePossibleEffects;
>> >      break;
>> >    }
>> >
>> > A volatile load is always a side-effect.
>>
>> Not for the purposes of unevaluated contexts though, is it? For instance:
>>
>> int * volatile x;
>> (void)sizeof(*x); // Should not warn, correct?
>
>
> Well, what about this:
>
>   volatile char *myMMIODevice = (char*)0xbadf00t;
>   sizeof(myMMIODevice[57]);
>
> We don't know why someone marked their type as volatile, but volatile
> accesses are one of the most unambiguously side-effecting things we have in
> C++.
>
> But I don't think this is a big deal either way; I don't think this is
> likely to have false positives or false negatives in practice.

I can see the argument either way, but I suspect in the case of a
volatile read, the expectation is that the read isn't side-effecting
(how many programmers truly remember that volatile reads can produce
side effects?). Then again, if you're using a volatile value, you
should be aware of the semantics.

I'll modify the test to treat it as side-effecting, with an
explanation. If we find it's chatty in practice and produces false
positives, we can modify the behavior at that time.

Thanks! I committed the patch in r224465 and will watch the bots for
any sign of troubles.

~Aaron

>
>> > @@ -3022,7 +3039,7 @@
>> >    case CXXTemporaryObjectExprClass: {
>> >      const CXXConstructExpr *CE = cast<CXXConstructExpr>(this);
>> >      if (!CE->getConstructor()->isTrivial())
>> > -      return true;
>> > +      return IncludePossibleEffects;
>> >      // A trivial constructor does not add any side-effects of its own.
>> > Just
>> > look
>> >      // at its arguments.
>> >      break;
>> >
>> > This should be
>> >
>> >   if (IncludePossibleEffects)
>> >     return true;
>> >
>> > There are a bunch of cases under
>> >
>> >     // FIXME: Classify these cases better.
>> >
>> > These are all "don't know" cases, and so should say something like:
>> >
>> >   if (IncludePossibleEffects)
>> >     return true;
>> >   break;
>> >
>> > rather than just
>> >
>> >   return true;
>> >
>> > (You can then put ObjCMessageExprClass and ObjCPropertyRefExprClass back
>> > where they were.)
>>
>> Good catches!
>>
>> > Otherwise, the patch LGTM; we can decide on whether this makes sense as
>> > an
>> > on-by-default warning once we get more experience with its false
>> > positive
>> > rate.
>>
>> Sounds good to me. I'll await your thoughts on the example I posted
>> above and whether it should warn or not before committing, since there
>> are test cases riding on the answer.
>>
>> Thanks!
>>
>> ~Aaron
>
>