[PATCH] [StaticAnalyzer] New checker Sizeof on expression

[Jordan Rose]

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?


Comments on the patch itself:

+  return sizeof(2 + 1);  // expected-warning{{Sizeof() on an expression with an operator may yield unexpected results}}

The rule that capitalizes analyzer output (unlike compiler warnings) makes this look very strange. Maybe it's worth inserting the word "Using" before "sizeof".


+//==- CheckSizeofOnExpression.cpp - Check expressions in sizeof() *- C++ -*-==//

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 -----==//


+//
+//  This file defines a check for expressions made in sizeof()
+//

Please turn this into a Doxygen file comment. http://llvm.org/docs/CodingStandards.html#file-headers


+class WalkAST : public StmtVisitor<WalkAST> {

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.)


+  Expr *Expr = E->getArgumentExpr();

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.


+  //Only check binary operators

+  // is expression based on macro => don't warn

Please use complete sentences for comments, including a space after the //.


+  BR.EmitBasicReport(AC->getDecl(),
+                     "Expression with an operation in sizeof()",
+                     "Logic",
+                     "Sizeof() on an expression with an operator "
+                     "may yield unexpected results.",
+                     ELoc, &R, 1);

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}.

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.)


+    walker.Visit(D->getBody());

If you do change to RecursiveASTVisitor, make sure you visit D, to include the constructor initializers.

Thanks for working on this!
Jordan


On Sep 20, 2013, at 2:37 , Anders Rönnholm <Anders.Ronnholm at evidente.se> wrote:

> Hi,
> 
> Sorry I broke a test with a last minute change.
> 
> Thanks,
> Anders
> 
> .......................................................................................................................
> Anders Rönnholm Senior Engineer
> Evidente ES East AB  Warfvinges väg 34  SE-112 51 Stockholm  Sweden
> 
> Mobile:                    +46 (0)70 912 42 54
> E-mail:                    Anders.Ronnholm at evidente.se
> 
> www.evidente.se
> 
> ________________________________________
> Från: Anders Rönnholm
> Skickat: den 20 september 2013 11:19
> Till: cfe-commits at cs.uiuc.edu
> Ämne: Re: [PATCH] [StaticAnalyzer] New checker Sizeof on expression
> 
> Hi,
> 
> I forgot that there is i line limit of 80, i have corrected that.
> 
> //Anders
> 
> .......................................................................................................................
> Anders Rönnholm Senior Engineer
> Evidente ES East AB  Warfvinges väg 34  SE-112 51 Stockholm  Sweden
> 
> Mobile:                    +46 (0)70 912 42 54
> E-mail:                    Anders.Ronnholm at evidente.se
> 
> www.evidente.se
> 
> ________________________________________
> Från: Anders Rönnholm
> Skickat: den 20 september 2013 10:23
> Till: cfe-commits at cs.uiuc.edu
> Ämne: [PATCH] [StaticAnalyzer] New checker Sizeof on expression
> 
> Hi,
> 
> 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.
> 
> Example:
> sizeof(2 + 1);
> 
> Thanks,
> Anders
> 
> .......................................................................................................................
> Anders Rönnholm Senior Engineer
> Evidente ES East AB  Warfvinges väg 34  SE-112 51 Stockholm  Sweden
> 
> Mobile:                    +46 (0)70 912 42 54
> E-mail:                    Anders.Ronnholm at evidente.se
> 
> www.evidente.se
-------------- next part --------------
A non-text attachment was scrubbed...
Name: sizeofonexpression.diff
Type: text/x-patch
Size: 5894 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130925/d280cc13/attachment.bin>
-------------- next part --------------
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits