[PATCH] Warning on unevaluated expression contexts with side effects

[Richard Smith]

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.

> @@ -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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20141217/35b2c8a7/attachment.html>