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
