Okay, the checker is committed in r194236. Thanks, Per! Jordan
On Nov 5, 2013, at 9:39 , Anna Zaks <[email protected]> wrote: > > On Nov 5, 2013, at 9:26 AM, Jordan Rose <[email protected]> wrote: > >> I got a chance to look at the results of analyzing LLVM/Clang with this >> checker, and also found zero reports—true or false positives. On the one >> hand, these mistakes probably don't last long in an open-source project. On >> the other, is it worth having a checker that catches problems that don't >> show up in the real world? > > I don't think it's fair to say that if the reports do not show in clang/llvm > codebase, the checker catches problems that don't show up in real world. > (Jordan probably misspoke.) > > This type of errors would probably be more common in newly written, not very > well tested code. Also, clang/llvm has been checked with other tools, which > might have exposed these types of bugs already. > >> Anna? >> >> (The answer is probably "yes", but I thought I might as well bring it up. >> Perhaps it's disabled in our "shallow" analysis mode.) > > I think this checker SHOULD be enabled in shallow mode. First, shallow is > mostly used when people want fast turnaround (as in running analyzer during > build). So there is a high chance the analyzer will be run on freshly written > code that, I think, is more likely to have bugs like these. Also, this is a > syntactic check; those are almost always fast enough for the analyzer. > > Anna. > >> Jordan >> >> >> On Oct 31, 2013, at 3:20 , Per Viberg <[email protected]> wrote: >> >>> >>> Hi Jordan, >>> >>> here is the new patch changed according to your comments below. I have >>> renamed the check from EqualCompareChecker to IdenticalExpChecker to >>> accommodate for upcoming tests in this category. I also ran the analysis on >>> an open source project called cppchecker to check performance. >>> >>> Without IdenticalExpChecker: 5197 seconds >>> With IdenticalExpChecker: 5239 seconds. >>> Result: approximately 40 seconds longer, which amounts to < 1% increase in >>> analyse time. >>> >>> Depending on the coding style of the source code analysed, this could >>> differ dramatically. If someone uses a lot of sub-expressions within >>> sub-expression the analysis time increases, but then this check becomes >>> more important to do since the error will be more difficult to detect by >>> manual review. >>> >>> The checker didn't find any "real" errors in the cppChecker project, and >>> not any false positives. >>> >>> best regards, >>> Per >>> ....................................................................................................................... >>> Per Viberg Senior Engineer >>> Evidente ES East AB Warfvinges väg 34 SE-112 51 Stockholm Sweden >>> Phone: +46 (0)8 402 79 00 >>> Mobile: +46 (0)70 912 42 52 >>> E-mail: [email protected] >>> >>> www.evidente.se >>> This e-mail, which might contain confidential information, is addressed to >>> the above stated person/company. If you are not the correct addressee, >>> employee or in any other way the person concerned, please notify the sender >>> immediately. At the same time, please delete this e-mail and destroy any >>> prints. Thank You. >>> Från: Jordan Rose [[email protected]] >>> Skickat: den 11 oktober 2013 02:43 >>> Till: Per Viberg >>> Cc: [email protected] >>> Ämne: Re: [PATCH][StaticAnalyzer] new check comparing equal expression >>> >>> Hi, Per. Looking good; more comments for you. The performance thing is also >>> something I'd like to get resolved soon: how long does this checker alone >>> take over a large body of code, compared to the existing path-sensitive >>> analysis? (It looks like it's linear on average, quadratic at worst, so I'd >>> expect it to be much cheaper, but still.) >>> >>> >>> +static bool isSameExpr(const ASTContext &Ctx, Expr *Expr1, Expr *Expr2); >>> >>> const Expr *, please? >>> >>> >>> + // only visit nodes that are binary expressions >>> >>> LLVM convention says that all comments should be complete sentences, >>> including capitalization and a final period. >>> >>> >>> + bool isSameExpression; >>> >>> No reason to declare this way ahead of time. Please move this down to where >>> it's actually used—or eliminate it altogether, and just put the call in the >>> if-condition. >>> >>> >>> + DeclRefExpr *declref1 = dyn_cast<DeclRefExpr>(LHS); >>> + DeclRefExpr *declref2 = dyn_cast<DeclRefExpr>(RHS); >>> >>> LLVM convention says that all variable names are UpperCamelCased. There are >>> some of these in isSameExpr as well. >>> >>> >>> + // If any side of comparison still has floatingpointrepresentation, >>> + // then it's an expression-> don't warn. >>> + // (here only LHS is checked since RHS will be implicit casted to >>> float) >>> >>> Please turn these into complete sentences (and split up "floating-point >>> representation"). >>> >>> >>> + } else { // No special case with floating point representation, report as >>> + // usual >>> + } >>> >>> I get that it's important to call out this case, but please put the comment >>> on its own line within the body of the if. >>> >>> >>> +// t*(u + t) and t*u + t*t are not considered equal. >>> >>> I wouldn't even worry about this. This is a pretty unlikely typo or >>> copy-paste error. Also, no reason not to make this whole block a proper >>> Doxygen comment. Even if it's not a public function, IDEs can still make >>> use of the comment if it's marked up as Doxygen (and with ///). >>> >>> >>> Has this checker found any real errors in code you have access to? Has it >>> found any false positives? >>> >>> Thanks for working on this! >>> Jordan >>> >>> >>> On Oct 10, 2013, at 8:42 , Per Viberg <[email protected]> wrote: >>> >>> > >>> > Hello, >>> > >>> > changed patch according to review comments. >>> > >>> > Please review revised patch. >>> > >>> > The patch adds a check that warns for comparison of equal expressions. >>> > example: >>> > x + 1 == x +1; >>> > >>> > best regards, >>> > Per >>> > >>> > ....................................................................................................................... >>> > Per Viberg Senior Engineer >>> > Evidente ES East AB Warfvinges väg 34 SE-112 51 Stockholm Sweden >>> > Phone: +46 (0)8 402 79 00 >>> > Mobile: +46 (0)70 912 42 52 >>> > E-mail: [email protected] >>> > >>> > www.evidente.se >>> > This e-mail, which might contain confidential information, is addressed >>> > to the above stated person/company. If you are not the correct addressee, >>> > employee or in any other way the person concerned, please notify the >>> > sender immediately. At the same time, please delete this e-mail and >>> > destroy any prints. Thank You. >>> > _______________________________________________ >>> > cfe-commits mailing list >>> > [email protected] >>> > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >>> >> <IdenticalExpr_rev3.diff> >>> _______________________________________________ >>> cfe-commits mailing list >>> [email protected] >>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >> >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
