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