On Wed, Dec 17, 2014 at 1:09 PM, Aaron Ballman <[email protected]> wrote:
> 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? 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 >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
