[PATCH] [StaticAnalyzer] New checker StringPlusChar

[Jordan Rose]

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 <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.
> 
> //Anders

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131001/b0e3989d/attachment.html>