[PATCH] [StaticAnalyzer] New checker Sizeof on expression
Anders Rönnholm
Anders.Ronnholm at evidente.se
Fri May 16 02:03:08 PDT 2014
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.
I have removed HasSideEffects now and modified the testfiles that started to trigg on the warning.
Also added extra parens to silence the warning.
//Anders
________________________________________
Från: Jordan Rose [jordan_rose at apple.com]
Skickat: den 13 maj 2014 18:38
Till: Anders Rönnholm
Cc: cfe-commits at cs.uiuc.edu; Daniel Marjamäki
Ämne: Re: [PATCH] [StaticAnalyzer] New checker Sizeof on expression
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:
+int SizeofFunctionCallExpression() {
+ return sizeof(SizeofDefine() - 1);
+} // no-warning
This should have a warning, since the function is not called. If it interferes with the VLA thing Aaron brought up, though...
I never got a response to this:
+ if (Binop->getLHS()->getType()->isArrayType() ||
+ Binop->getLHS()->getType()->isAnyPointerType() ||
+ Binop->getRHS()->getType()->isArrayType() ||
+ Binop->getRHS()->getType()->isAnyPointerType())
+ return;
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.
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.
Jordan
On May 13, 2014, at 3:27 , Anders Rönnholm <Anders.Ronnholm at evidente.se<mailto:Anders.Ronnholm at evidente.se>> wrote:
Pinging
________________________________________
Från: Anders Rönnholm
Skickat: den 27 mars 2014 11:09
Till: Jordan Rose
Cc: cfe-commits at cs.uiuc.edu<mailto:cfe-commits at cs.uiuc.edu>; Daniel Marjamäki
Ämne: SV: [PATCH] [StaticAnalyzer] New checker Sizeof on expression
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.
I removed the check for HasSideEffects previously but had to take back. I noticed that the patch triggered some false positives without it.
//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<mailto:Anders.Ronnholm at evidente.se>
www.evidente.se<http://www.evidente.se>
________________________________________
Från: Jordan Rose [jordan_rose at apple.com]
Skickat: den 31 januari 2014 18:50
Till: Anders Rönnholm
Cc: cfe-commits at cs.uiuc.edu; Daniel Marjamäki
Ämne: Re: [PATCH] [StaticAnalyzer] New checker Sizeof on expression
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.
"operand of sizeof is a binary expression of type %0, which may not be intended"
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?
What do you think?
Jordan
On Jan 23, 2014, at 6:40 , Anders Rönnholm <Anders.Ronnholm at evidente.se<mailto:Anders.Ronnholm at evidente.se>> wrote:
Hi,
New one with comments handled.
________________________________________
Från: Jordan Rose [jordan_rose at apple.com<mailto:jordan_rose at apple.com>]
Skickat: den 20 december 2013 19:15
Till: Anders Rönnholm
Cc: cfe-commits at cs.uiuc.edu<mailto:cfe-commits at cs.uiuc.edu>; Daniel Marjamäki; Anna Zaks; David Blaikie; Richard Smith; Matt Calabrese
Ämne: Re: [PATCH] [StaticAnalyzer] New checker Sizeof on expression
On Dec 10, 2013, at 4:38 , Anders Rönnholm <Anders.Ronnholm at evidente.se<mailto:Anders.Ronnholm at evidente.se><mailto:Anders.Ronnholm at evidente.se>> wrote:
Are you OK to commit this patch or do you see more issues?
I'm not sure if anyone else has ideological concerns. There's always a flag to turn this off, I suppose.
+ if (S.isSFINAEContext())
+ return;
Code style: extra indent?
+ if(E->HasSideEffects(S.getASTContext()))
+ return;
sizeof doesn't evaluate its argument, so I'm not sure why you wouldn't want to warn here.
+ const FunctionDecl *FD = S.getCurFunctionDecl();
+ if(FD && FD->isFunctionTemplateSpecialization())
+ return;
Code style: space after if. (Above too, actually.)
+ if (Binop->getLHS()->getType()->isArrayType() ||
+ Binop->getLHS()->getType()->isAnyPointerType() ||
+ Binop->getRHS()->getType()->isArrayType() ||
+ Binop->getRHS()->getType()->isAnyPointerType())
+ return;
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.
+def warn_sizeof_bin_op : Warning<
+ "using sizeof() on an expression with an operator may yield unexpected results">,
+ InGroup<SizeofOnExpression>;
+
+def warn_sizeof_sizeof : Warning<
+ "using sizeof() on sizeof() may yield unexpected results.">,
+ InGroup<SizeofOnExpression>;
+
sizeof doesn't actually require parens, so we shouldn't put the parens in the diagnostics.
<sizeofonexpression.diff>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: sizeofonexpression.diff
Type: text/x-patch
Size: 14160 bytes
Desc: sizeofonexpression.diff
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140516/a412f170/attachment.bin>
More information about the cfe-commits
mailing list