Thanks Jordan! You have two comments that I'd like to comment on.
>> + // Only check add operators. >> + if (B->getOpcode() != BO_Add) > > How about +=? Yes. It sounds good with +=. Is it ok if we keep this patch as it is.. for simplicitys sake. After this patch is accepted we can add a new patch with a few improvements. I also have in mind to extend it so it warns about CharPlusString. I am hoping it is fine to wait with that too. >> + const CharacterLiteral *CharExpr = >> + dyn_cast<CharacterLiteral>(B->getRHS()->IgnoreImpCasts()); > > Should this be a character literal, or just any expression with "char" type? > I realize that someone could be using a char-sized variable as an index, but > that seems fairly rare, and harder to catch by hand. Using char-sized variable as index is not that rare imho. I especially think that is true when you are in a environment when you dont use 'char', 'int', etc but use s8,s16,s32 instead. as you know this is not too uncommon. if you normally use s8 instead of s32 when you know the value is small then some index variables will be s8.. Best regards, Daniel Marjamäki .................................................................................................................. Daniel Marjamäki Senior Engineer Evidente ES East AB Warfvinges väg 34 SE-112 51 Stockholm Sweden Mobile: +46 (0)709 12 42 62 E-mail: [email protected] www.evidente.se Från: [email protected] [[email protected]] för Jordan Rose [[email protected]] Skickat: den 1 oktober 2013 18:32 Till: Anders Rönnholm Cc: [email protected] Ämne: Re: [PATCH] [StaticAnalyzer] New checker StringPlusChar The categories are supposed to be very broad groupings: "Logic error", "Memory leak", "Convention X". "Logic error" is probably fine here, even though it's generic, since there's not really a group that fits this general kind of mistake. I just want to avoid magic strings—we shouldn't have variants of the same thing because they'll show up as different categories. Comments on the patch: + // Only check add operators. + if (B->getOpcode() != BO_Add) How about +=? + const CharacterLiteral *CharExpr = + dyn_cast<CharacterLiteral>(B->getRHS()->IgnoreImpCasts()); Should this be a character literal, or just any expression with "char" type? I realize that someone could be using a char-sized variable as an index, but that seems fairly rare, and harder to catch by hand. + if (!(StringRefExpr->getType() == + Context.getPointerType(Context.getConstType(Context.CharTy)) || + StringRefExpr->getType() == Context.getPointerType(Context.CharTy))) Things to consider: 1. Do explicitly signed or unsigned chars count? In that case, you should use Type::isCharType(). 2. Do wide chars count? Then you should use Type::isAnyCharacterType(). 3. Even if none of the above, you should consider typedefs, so it's still worth going through getCanonicalType() and getPointeeType() rather than building up a new pointer type. One possibility: StringRefExpr->getType()->getPointeeType()->isAnyCharacterType(). At this point, though, StringRefExpr->getType() probably deserves a helper variable. +// RUN: %clang_cc1 -analyze -analyzer-checker=alpha.core.StringPlusChar -verify %s Please include the "core" package in these checks; running without core enabled isn't a use case we're interested in. Jordan On Sep 30, 2013, at 3:25 , Anders Rönnholm <[email protected]> wrote: Hi Jordan, Thank you for your comments. I have changed the check to use RecursiveASTVisitor. However i'm not sure what i should write as category in bugreport. You mentioned adding a new category in CommonBugCategories for SIzeofOnExpression, should i do that or is what i have enough for now? I have provided a new diff. //Anders _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
