<div dir="ltr">I'm worried about this having significant numbers of false positives, especially for an on-by-default warning. Has this been run against any large codebases (particularly C++ ones)?<div><br></div><div><div>
+ if (S.isSFINAEContext())</div><div>+ return;</div><div>+</div></div><div><div>+ const FunctionDecl *FD = S.getCurFunctionDecl();</div><div>+ if (FD && FD->isFunctionTemplateSpecialization())</div><div>+ return;</div>
</div><div><br></div><div>This looks suspicious -- we should suppress this for any template instantiation, not just function templates. Check ActiveTemplateInstantiations.empty() instead.</div><div><br></div><div><div>+ if (!Binop->isMultiplicativeOp() &&</div>
<div>+ !Binop->isAdditiveOp())</div><div>+ return;</div></div><div><br></div><div>I find this a little surprising. I'd think we should warn on this sort of thing:</div><div><br></div><div> bool b = sizeof(a > 4);</div>
<div><br></div><div>... but I don't want to block the patch on potential new features.</div><div><br></div><div><div>+ << FixItHint::CreateInsertion(Binop->getLHS()->getLocStart(), "&")</div>
<div>+ << FixItHint::CreateReplacement(SourceRange(Binop->getOperatorLoc()), "[")</div><div>+ << FixItHint::CreateInsertion(EndLoc, "]");</div></div><div><br></div><div>
This fixit looks wrong (both times).</div><div><br></div><div>+ if (const ParenExpr *PE = dyn_cast<ParenExpr>(E)) {<br></div><div>//...</div><div><div>+ } else if (const UnaryExprOrTypeTraitExpr *UnaryExpr =</div>
<div>+ dyn_cast<UnaryExprOrTypeTraitExpr>(PE->getSubExpr())) {</div><div>+ if (UnaryExpr->getKind() != UETT_SizeOf)</div><div>+ return;</div></div><div><br></div><div>Is it intentional that we warn on 'sizeof (sizeof(x))' but not on 'sizeof sizeof(x)'?</div>
</div><div class="gmail_extra"><br><br><div class="gmail_quote">On Fri, May 16, 2014 at 9:06 AM, Jordan Rose <span dir="ltr"><<a href="mailto:jordan_rose@apple.com" target="_blank">jordan_rose@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">This looks good to me. Richard, anyone else, any additional comments?<br>
<br>
Jordan<br>
<div><div class="h5"><br>
<br>
On May 16, 2014, at 2:03 , Anders Rönnholm <<a href="mailto:Anders.Ronnholm@evidente.se">Anders.Ronnholm@evidente.se</a>> wrote:<br>
<br>
> Without HasSideEffects you get lots of warnings in templates. From what i remember there were some discussion about not warning in templates but i might remember wrong, it's been a while now.<br>
><br>
> I have removed HasSideEffects now and modified the testfiles that started to trigg on the warning.<br>
><br>
> Also added extra parens to silence the warning.<br>
><br>
> //Anders<br>
><br>
> ________________________________________<br>
> Från: Jordan Rose [<a href="mailto:jordan_rose@apple.com">jordan_rose@apple.com</a>]<br>
> Skickat: den 13 maj 2014 18:38<br>
> Till: Anders Rönnholm<br>
> Cc: <a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a>; Daniel Marjamäki<br>
> Ämne: Re: [PATCH] [StaticAnalyzer] New checker Sizeof on expression<br>
><br>
> 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:<br>
><br>
> +int SizeofFunctionCallExpression() {<br>
> + return sizeof(SizeofDefine() - 1);<br>
> +} // no-warning<br>
><br>
> This should have a warning, since the function is not called. If it interferes with the VLA thing Aaron brought up, though...<br>
><br>
> I never got a response to this:<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>
> Finally, how about using an extra set of parens to silence the warning? It's harder to typo, and we have some precedent for that.<br>
><br>
> Jordan<br>
><br>
><br>
> On May 13, 2014, at 3:27 , Anders Rönnholm <<a href="mailto:Anders.Ronnholm@evidente.se">Anders.Ronnholm@evidente.se</a><mailto:<a href="mailto:Anders.Ronnholm@evidente.se">Anders.Ronnholm@evidente.se</a>>> wrote:<br>
><br>
> 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><mailto:<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: <a href="tel:%2B46%20%280%2970%20912%2042%2054" value="+46709124254">+46 (0)70 912 42 54</a><br>
> E-mail: <a href="mailto:Anders.Ronnholm@evidente.se">Anders.Ronnholm@evidente.se</a><mailto:<a href="mailto:Anders.Ronnholm@evidente.se">Anders.Ronnholm@evidente.se</a>><br>
><br>
> <a href="http://www.evidente.se" target="_blank">www.evidente.se</a><<a href="http://www.evidente.se" target="_blank">http://www.evidente.se</a>><br>
><br>
> ________________________________________<br>
> Från: Jordan Rose [<a href="mailto:jordan_rose@apple.com">jordan_rose@apple.com</a>]<br>
> Skickat: den 31 januari 2014 18:50<br>
> Till: Anders Rönnholm<br>
> Cc: <a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a>; 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 <<a href="mailto:Anders.Ronnholm@evidente.se">Anders.Ronnholm@evidente.se</a><mailto:<a href="mailto:Anders.Ronnholm@evidente.se">Anders.Ronnholm@evidente.se</a>>> wrote:<br>
><br>
> 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><mailto:<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: <a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><mailto:<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><mailto:<a href="mailto:Anders.Ronnholm@evidente.se">Anders.Ronnholm@evidente.se</a>><mailto:<a href="mailto:Anders.Ronnholm@evidente.se">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>
><br>
> <sizeofonexpression.diff><br>
</div></div>> <sizeofonexpression.diff><br>
<br>
</blockquote></div><br></div>