<html><head><meta http-equiv="Content-Type" content="text/html charset=iso-8859-1"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><div>Sorry for letting this slip through the cracks! I know it's now been a month and a half, but what were the false positives you saw without the HasSideEffects check? For example:</div><div><br></div><div><div>+int SizeofFunctionCallExpression() {</div><div>+ return sizeof(SizeofDefine() - 1);</div><div>+} // no-warning</div></div><div><br></div><div>This should have a warning, since the function is not called. If it interferes with the VLA thing Aaron brought up, though...</div><div><br></div><div>I never got a response to this:</div><div><br></div><div><blockquote type="cite"><span style="font-family: Monaco; font-size: 11px;">+ if (Binop->getLHS()->getType()->isArrayType() ||</span><br style="font-family: Monaco; font-size: 11px;"><span style="font-family: Monaco; font-size: 11px;">+ Binop->getLHS()->getType()->isAnyPointerType() ||</span><br style="font-family: Monaco; font-size: 11px;"><span style="font-family: Monaco; font-size: 11px;">+ Binop->getRHS()->getType()->isArrayType() ||</span><br style="font-family: Monaco; font-size: 11px;"><span style="font-family: Monaco; font-size: 11px;">+ Binop->getRHS()->getType()->isAnyPointerType())</span><br style="font-family: Monaco; font-size: 11px;"><span style="font-family: Monaco; font-size: 11px;">+ return;</span><br style="font-family: Monaco; font-size: 11px;"></blockquote></div><div><blockquote type="cite"><br></blockquote><blockquote type="cite">I don't think this is correct...the user is only trying to get ptrdiff_t if <i>both</i> the LHS and RHS are pointer-ish.<br></blockquote><br></div><div>Finally, how about using an <i>extra</i> set of parens to silence the warning? It's harder to typo, and we have some precedent for that. </div><div><br></div><div>Jordan</div><div><br></div><br><div><div>On May 13, 2014, at 3:27 , Anders Rönnholm <<a href="mailto:Anders.Ronnholm@evidente.se">Anders.Ronnholm@evidente.se</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite">Pinging<br>________________________________________<br>Från: Anders Rönnholm<br>Skickat: den 27 mars 2014 11:09<br>Till: Jordan Rose<br>Cc: <a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a>; Daniel Marjamäki<br>Ämne: SV: [PATCH] [StaticAnalyzer] New checker Sizeof on expression<br><br>New patch with new diagnostic message. I couldn't come up with a better wording so i'm using your suggestion. I don't know of a good way to silence the warning.<br><br>I removed the check for HasSideEffects previously but had to take back. I noticed that the patch triggered some false positives without it.<br><br>//Anders<br><br>.......................................................................................................................<br>Anders Rönnholm Senior Engineer<br>Evidente ES East AB Warfvinges väg 34 SE-112 51 Stockholm Sweden<br><br>Mobile: +46 (0)70 912 42 54<br>E-mail: <a href="mailto:Anders.Ronnholm@evidente.se">Anders.Ronnholm@evidente.se</a><br><br><a href="http://www.evidente.se">www.evidente.se</a><br><br>________________________________________<br>Från: Jordan Rose [jordan_rose@apple.com]<br>Skickat: den 31 januari 2014 18:50<br>Till: Anders Rönnholm<br>Cc: cfe-commits@cs.uiuc.edu; Daniel Marjamäki<br>Ämne: Re: [PATCH] [StaticAnalyzer] New checker Sizeof on expression<br><br>Sorry to have let this slip! This is looking good, but I had one more thought about the diagnostic message. It says "may yield unexpected results", but doesn't really explain what those unexpected results are. I was wondering if we could work the type into the message for the operator case.<br><br>"operand of sizeof is a binary expression of type %0, which may not be intended"<br><br>I don't like that wording either, but at least this one makes people say "what? why isn't it [the type I actually want]?". Also, should there be a way to silence the warning?<br><br>What do you think?<br>Jordan<br><br><br>On Jan 23, 2014, at 6:40 , Anders Rönnholm <Anders.Ronnholm@evidente.se<mailto:Anders.Ronnholm@evidente.se>> wrote:<br><br>Hi,<br><br>New one with comments handled.<br><br>________________________________________<br>Från: Jordan Rose [jordan_rose@apple.com<mailto:jordan_rose@apple.com>]<br>Skickat: den 20 december 2013 19:15<br>Till: Anders Rönnholm<br>Cc: cfe-commits@cs.uiuc.edu<mailto:cfe-commits@cs.uiuc.edu>; Daniel Marjamäki; Anna Zaks; David Blaikie; Richard Smith; Matt Calabrese<br>Ämne: Re: [PATCH] [StaticAnalyzer] New checker Sizeof on expression<br><br>On Dec 10, 2013, at 4:38 , Anders Rönnholm <Anders.Ronnholm@evidente.se<mailto:Anders.Ronnholm@evidente.se><mailto:Anders.Ronnholm@evidente.se>> wrote:<br><br>Are you OK to commit this patch or do you see more issues?<br><br>I'm not sure if anyone else has ideological concerns. There's always a flag to turn this off, I suppose.<br><br><br>+ if (S.isSFINAEContext())<br>+ return;<br><br>Code style: extra indent?<br><br><br>+ if(E->HasSideEffects(S.getASTContext()))<br>+ return;<br><br>sizeof doesn't evaluate its argument, so I'm not sure why you wouldn't want to warn here.<br><br><br>+ const FunctionDecl *FD = S.getCurFunctionDecl();<br>+ if(FD && FD->isFunctionTemplateSpecialization())<br>+ return;<br><br>Code style: space after if. (Above too, actually.)<br><br><br>+ if (Binop->getLHS()->getType()->isArrayType() ||<br>+ Binop->getLHS()->getType()->isAnyPointerType() ||<br>+ Binop->getRHS()->getType()->isArrayType() ||<br>+ Binop->getRHS()->getType()->isAnyPointerType())<br>+ return;<br><br>I don't think this is correct...the user is only trying to get ptrdiff_t if both the LHS and RHS are pointer-ish.<br><br><br>+def warn_sizeof_bin_op : Warning<<br>+ "using sizeof() on an expression with an operator may yield unexpected results">,<br>+ InGroup<SizeofOnExpression>;<br>+<br>+def warn_sizeof_sizeof : Warning<<br>+ "using sizeof() on sizeof() may yield unexpected results.">,<br>+ InGroup<SizeofOnExpression>;<br>+<br><br>sizeof doesn't actually require parens, so we shouldn't put the parens in the diagnostics.<br><br><span><sizeofonexpression.diff></span></blockquote></div><br></body></html>