[PATCH] Warning on unevaluated expression contexts with side effects

[Richard Trieu]

I am seeing a lot of new diagnostics from the evaluated in a typeid
warning.  I think that it should be split off to its own sub-group.

On Wed, Dec 17, 2014 at 1:58 PM, Aaron Ballman <aaron at aaronballman.com>
wrote:

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