SV: [PATCH] [StaticAnalyzer] New checker StringPlusChar
Daniel.Marjamaki at evidente.se
Tue Oct 1 19:58:17 PDT 2013
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..
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: Daniel.Marjamaki at evidente.se
Från: cfe-commits-bounces at cs.uiuc.edu [cfe-commits-bounces at cs.uiuc.edu] för Jordan Rose [jordan_rose at apple.com]
Skickat: den 1 oktober 2013 18:32
Till: Anders Rönnholm
Cc: cfe-commits at cs.uiuc.edu
Ä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 =
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.
On Sep 30, 2013, at 3:25 , Anders Rönnholm <Anders.Ronnholm at evidente.se> wrote:
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?
have provided a new diff.
More information about the cfe-commits