<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Wed, Sep 25, 2013 at 6:39 PM, 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">Seems reasonable. This is something that we could also implement as a compiler warning, though. David, Richard, Eli, what do you think: compiler or analyzer?<br>
</blockquote><div><br></div><div>personally I think anything cheap enough to do at compile time probably should be, but Doug's general attitude has been that the other prerequisite is bug-finding capability; Essentially if a warning would be too noisy to be on by default in an existing codebase (because it caused more busy work than bug fixing to cleanup the codebase to be warning-free) then it doesn't go in the compiler<br>
<br>Putting it in the analyzer would then seem an unfortunate side-effect of its true/false positive rate.<br><br>Of course we're still missing the middle ground: things that don't have the true positive rate to be intrinsic features of the compiler, but a codebase may want to opt-in to if the bug-finding is acceptable to them. This would come in the form of a Clang plugin, ideally, but we don't have an authoritative/configurable plugin for this purpose yet.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<br>
Comments on the patch itself:<br>
<br>
+ return sizeof(2 + 1); // expected-warning{{Sizeof() on an expression with an operator may yield unexpected results}}<br>
<br>
The rule that capitalizes analyzer output (unlike compiler warnings) makes this look very strange. Maybe it's worth inserting the word "Using" before "sizeof".<br>
<br>
<br>
+//==- CheckSizeofOnExpression.cpp - Check expressions in sizeof() *- C++ -*-==//<br>
<br>
The format header "-*- C++ -*-" needs all those dashes and stars to be recognized. However, since this is a .cpp file, you should just leave it out anyway and uses dashes to fill the empty space -----==//<br>
<br>
<br>
+//<br>
+// This file defines a check for expressions made in sizeof()<br>
+//<br>
<br>
Please turn this into a Doxygen file comment. <a href="http://llvm.org/docs/CodingStandards.html#file-headers" target="_blank">http://llvm.org/docs/CodingStandards.html#file-headers</a><br>
<br>
<br>
+class WalkAST : public StmtVisitor<WalkAST> {<br>
<br>
Other checkers predate RecursiveASTVisitor, but you might as well use RecursiveASTVisitor and avoid writing the VisitChildren boilerplate. This would also handle checking C++ constructor initializers, which aren't actually part of the body of the constructor. (We should probably migrate all the old walkers to RecursiveASTVisitor for this reason.)<br>
<br>
<br>
+ Expr *Expr = E->getArgumentExpr();<br>
<br>
We try not to shadow type names with local variable names, which I know is a bit trickier when variable names are also capitalized. Also, there's no reason not to make this a pointer to const.<br>
<br>
<br>
+ //Only check binary operators<br>
<br>
+ // is expression based on macro => don't warn<br>
<br>
Please use complete sentences for comments, including a space after the //.<br>
<br>
<br>
+ BR.EmitBasicReport(AC->getDecl(),<br>
+ "Expression with an operation in sizeof()",<br>
+ "Logic",<br>
+ "Sizeof() on an expression with an operator "<br>
+ "may yield unexpected results.",<br>
+ ELoc, &R, 1);<br>
<br>
The bug categories aren't well-defined, but there's already one called "Logic error" that's used by some of the analyzer core checkers. It might be nice to refactor this, SizeofPointerChecker, and BuiltinBug to share a single new constant in CommonBugCategories.{h,cpp}.<br>
<br>
Also, there's an overload that takes a single SourceRange. (Double-also, BugReporter should just be using ArrayRef, which autoconverts from a single element anyway.)<br>
<br>
<br>
+ walker.Visit(D->getBody());<br>
<br>
If you do change to RecursiveASTVisitor, make sure you visit D, to include the constructor initializers.<br>
<br>
Thanks for working on this!<br>
<span class="HOEnZb"><font color="#888888">Jordan<br>
</font></span><div class="HOEnZb"><div class="h5"><br>
<br>
On Sep 20, 2013, at 2:37 , Anders Rönnholm <<a href="mailto:Anders.Ronnholm@evidente.se">Anders.Ronnholm@evidente.se</a>> wrote:<br>
<br>
> Hi,<br>
><br>
> Sorry I broke a test with a last minute change.<br>
><br>
> Thanks,<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><br>
><br>
> <a href="http://www.evidente.se" target="_blank">www.evidente.se</a><br>
><br>
> ________________________________________<br>
> Från: Anders Rönnholm<br>
> Skickat: den 20 september 2013 11:19<br>
> Till: <a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
> Ämne: Re: [PATCH] [StaticAnalyzer] New checker Sizeof on expression<br>
><br>
> Hi,<br>
><br>
> I forgot that there is i line limit of 80, i have corrected that.<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><br>
><br>
> <a href="http://www.evidente.se" target="_blank">www.evidente.se</a><br>
><br>
> ________________________________________<br>
> Från: Anders Rönnholm<br>
> Skickat: den 20 september 2013 10:23<br>
> Till: <a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
> Ämne: [PATCH] [StaticAnalyzer] New checker Sizeof on expression<br>
><br>
> Hi,<br>
><br>
> I have another checker i would like to get reviewed. It checks for usage of sizeof on an expression. For now it only checks binary operators. Sizeof on defines does not generate a warning.<br>
><br>
> Example:<br>
> sizeof(2 + 1);<br>
><br>
> Thanks,<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><br>
><br>
> <a href="http://www.evidente.se" target="_blank">www.evidente.se</a><br>
</div></div><br>> _______________________________________________<br>
> cfe-commits mailing list<br>
> <a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
<br>
<br></blockquote></div><br></div></div>