On Wed, Dec 17, 2014 at 2:49 PM, Richard Smith <[email protected]> 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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
