<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Wed, Dec 17, 2014 at 1:09 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:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><span class="">On Wed, Dec 17, 2014 at 2:49 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk">richard@metafoo.co.uk</a>> 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>
</span>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?</blockquote><div><br></div><div>Well, what about this:</div><div><br></div><div>  volatile char *myMMIODevice = (char*)0xbadf00t;</div><div>  sizeof(myMMIODevice[57]);<br></div><div><br></div><div>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++.</div><div><br></div><div>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.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><span class="">
> @@ -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. 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>
</span>Good catches!<br>
<span class=""><br>
> Otherwise, the patch LGTM; we can decide on whether this makes sense as an<br>
> on-by-default warning once we get more experience with its false positive<br>
> rate.<br>
<br>
</span>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>
<span class=""><font color="#888888"><br>
~Aaron<br>
</font></span></blockquote></div><br></div></div>