<div dir="ltr">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.</div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Dec 17, 2014 at 1:58 PM, Aaron Ballman <span dir="ltr"><<a href="mailto:aaron@aaronballman.com" target="_blank">aaron@aaronballman.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">On Wed, Dec 17, 2014 at 4:28 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk">richard@metafoo.co.uk</a>> wrote:<br>
> On Wed, Dec 17, 2014 at 1:09 PM, Aaron Ballman <<a href="mailto:aaron@aaronballman.com">aaron@aaronballman.com</a>><br>
> wrote:<br>
>><br>
>> On Wed, Dec 17, 2014 at 2:49 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk">richard@metafoo.co.uk</a>><br>
>> wrote:<br>
>> > @@ -3009,7 +3026,7 @@<br>
>> >      const CastExpr *CE = cast<CastExpr>(this);<br>
>> >      if (CE->getCastKind() == CK_LValueToRValue &&<br>
>> >          CE->getSubExpr()->getType().isVolatileQualified())<br>
>> > -      return true;<br>
>> > +      return IncludePossibleEffects;<br>
>> >      break;<br>
>> >    }<br>
>> ><br>
>> > A volatile load is always a side-effect.<br>
>><br>
>> Not for the purposes of unevaluated contexts though, is it? For instance:<br>
>><br>
>> int * volatile x;<br>
>> (void)sizeof(*x); // Should not warn, correct?<br>
><br>
><br>
> Well, what about this:<br>
><br>
>   volatile char *myMMIODevice = (char*)0xbadf00t;<br>
>   sizeof(myMMIODevice[57]);<br>
><br>
> We don't know why someone marked their type as volatile, but volatile<br>
> accesses are one of the most unambiguously side-effecting things we have in<br>
> C++.<br>
><br>
> But I don't think this is a big deal either way; I don't think this is<br>
> likely to have false positives or false negatives in practice.<br>
<br>
</span>I can see the argument either way, but I suspect in the case of a<br>
volatile read, the expectation is that the read isn't side-effecting<br>
(how many programmers truly remember that volatile reads can produce<br>
side effects?). Then again, if you're using a volatile value, you<br>
should be aware of the semantics.<br>
<br>
I'll modify the test to treat it as side-effecting, with an<br>
explanation. If we find it's chatty in practice and produces false<br>
positives, we can modify the behavior at that time.<br>
<br>
Thanks! I committed the patch in r224465 and will watch the bots for<br>
any sign of troubles.<br>
<span class="HOEnZb"><font color="#888888"><br>
~Aaron<br>
</font></span><div class="HOEnZb"><div class="h5"><br>
><br>
>> > @@ -3022,7 +3039,7 @@<br>
>> >    case CXXTemporaryObjectExprClass: {<br>
>> >      const CXXConstructExpr *CE = cast<CXXConstructExpr>(this);<br>
>> >      if (!CE->getConstructor()->isTrivial())<br>
>> > -      return true;<br>
>> > +      return IncludePossibleEffects;<br>
>> >      // A trivial constructor does not add any side-effects of its own.<br>
>> > Just<br>
>> > look<br>
>> >      // at its arguments.<br>
>> >      break;<br>
>> ><br>
>> > This should be<br>
>> ><br>
>> >   if (IncludePossibleEffects)<br>
>> >     return true;<br>
>> ><br>
>> > There are a bunch of cases under<br>
>> ><br>
>> >     // FIXME: Classify these cases better.<br>
>> ><br>
>> > These are all "don't know" cases, and so should say something like:<br>
>> ><br>
>> >   if (IncludePossibleEffects)<br>
>> >     return true;<br>
>> >   break;<br>
>> ><br>
>> > rather than just<br>
>> ><br>
>> >   return true;<br>
>> ><br>
>> > (You can then put ObjCMessageExprClass and ObjCPropertyRefExprClass back<br>
>> > where they were.)<br>
>><br>
>> Good catches!<br>
>><br>
>> > Otherwise, the patch LGTM; we can decide on whether this makes sense as<br>
>> > an<br>
>> > on-by-default warning once we get more experience with its false<br>
>> > positive<br>
>> > rate.<br>
>><br>
>> Sounds good to me. I'll await your thoughts on the example I posted<br>
>> above and whether it should warn or not before committing, since there<br>
>> are test cases riding on the answer.<br>
>><br>
>> Thanks!<br>
>><br>
>> ~Aaron<br>
><br>
><br>
</div></div></blockquote></div><br></div>