<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 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.</div><div><br></div><div>"operand of sizeof is a binary expression of type %0, which may not be intended"</div><div><br></div><div>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?</div><div><br></div><div>What do you think?</div><div>Jordan</div><div><br></div><br><div><div>On Jan 23, 2014, at 6:40 , 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"><div style="font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;">Hi,<br><br>New one with comments handled.<br><br>________________________________________<br>Från: Jordan Rose [<a href="mailto:jordan_rose@apple.com">jordan_rose@apple.com</a>]<br>Skickat: den 20 december 2013 19:15<br>Till: Anders Rönnholm<br>Cc:<span class="Apple-converted-space"> </span><a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a>; 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 <<a href="mailto:Anders.Ronnholm@evidente.se">Anders.Ronnholm@evidente.se</a><<a href="mailto:Anders.Ronnholm@evidente.se">mailto:Anders.Ronnholm@evidente.se</a>>> 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></div></blockquote></div></body></html>