[PATCH] Warning on unevaluated expression contexts with side effects

[Aaron Ballman]

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?

>
> @@ -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