[PATCH] [StaticAnalyzer] New checker StringPlusChar
jordan_rose at apple.com
Tue Oct 1 09:32:48 PDT 2013
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:
> 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.
-------------- next part --------------
An HTML attachment was scrubbed...
More information about the cfe-commits