I have run it against LLVM and Libreoffice with no false positives. No real bugs found either.
//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: [email protected] www.evidente.se ________________________________________ Från: [email protected] [[email protected]] för Richard Smith [[email protected]] Skickat: den 16 maj 2014 20:07 Till: Jordan Rose Cc: Anders Rönnholm; [email protected]; Daniel Marjamäki Ämne: Re: [PATCH] [StaticAnalyzer] New checker Sizeof on expression 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)? + if (S.isSFINAEContext()) + return; + + const FunctionDecl *FD = S.getCurFunctionDecl(); + if (FD && FD->isFunctionTemplateSpecialization()) + return; This looks suspicious -- we should suppress this for any template instantiation, not just function templates. Check ActiveTemplateInstantiations.empty() instead. + if (!Binop->isMultiplicativeOp() && + !Binop->isAdditiveOp()) + return; I find this a little surprising. I'd think we should warn on this sort of thing: bool b = sizeof(a > 4); ... but I don't want to block the patch on potential new features. + << FixItHint::CreateInsertion(Binop->getLHS()->getLocStart(), "&") + << FixItHint::CreateReplacement(SourceRange(Binop->getOperatorLoc()), "[") + << FixItHint::CreateInsertion(EndLoc, "]"); This fixit looks wrong (both times). + if (const ParenExpr *PE = dyn_cast<ParenExpr>(E)) { //... + } else if (const UnaryExprOrTypeTraitExpr *UnaryExpr = + dyn_cast<UnaryExprOrTypeTraitExpr>(PE->getSubExpr())) { + if (UnaryExpr->getKind() != UETT_SizeOf) + return; Is it intentional that we warn on 'sizeof (sizeof(x))' but not on 'sizeof sizeof(x)'? On Fri, May 16, 2014 at 9:06 AM, Jordan Rose <[email protected]<mailto:[email protected]>> wrote: This looks good to me. Richard, anyone else, any additional comments? Jordan On May 16, 2014, at 2:03 , Anders Rönnholm <[email protected]<mailto:[email protected]>> wrote: > 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 [[email protected]<mailto:[email protected]>] > Skickat: den 13 maj 2014 18:38 > Till: Anders Rönnholm > Cc: [email protected]<mailto:[email protected]>; 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 > <[email protected]<mailto:[email protected]><mailto:[email protected]<mailto:[email protected]>>> > wrote: > > Pinging > ________________________________________ > Från: Anders Rönnholm > Skickat: den 27 mars 2014 11:09 > Till: Jordan Rose > Cc: > [email protected]<mailto:[email protected]><mailto:[email protected]<mailto:[email protected]>>; > 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<tel:%2B46%20%280%2970%20912%2042%2054> > E-mail: > [email protected]<mailto:[email protected]><mailto:[email protected]<mailto:[email protected]>> > > www.evidente.se<http://www.evidente.se><http://www.evidente.se> > > ________________________________________ > Från: Jordan Rose [[email protected]<mailto:[email protected]>] > Skickat: den 31 januari 2014 18:50 > Till: Anders Rönnholm > Cc: [email protected]<mailto:[email protected]>; 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 > <[email protected]<mailto:[email protected]><mailto:[email protected]<mailto:[email protected]>>> > wrote: > > Hi, > > New one with comments handled. > > ________________________________________ > Från: Jordan Rose > [[email protected]<mailto:[email protected]><mailto:[email protected]<mailto:[email protected]>>] > Skickat: den 20 december 2013 19:15 > Till: Anders Rönnholm > Cc: > [email protected]<mailto:[email protected]><mailto:[email protected]<mailto:[email protected]>>; > 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 > <[email protected]<mailto:[email protected]><mailto:[email protected]<mailto:[email protected]>><mailto:[email protected]<mailto:[email protected]>>> > 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> > <sizeofonexpression.diff> _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
