Looking pretty good, though Richard should probably give final approval now. Is
it worth checking that the string is actually a pointer to some kind of
character, though? It's unlikely that you'd add a character to an int*, but the
message is a bit off, then.
Also, the type in the message should be the type of the character literal (i.e.
L'a', u'a', and U'a'), not a hardcoded "char". Unfortunately, in C that type is
'int', so in the C case you should check and see if the value fits in a char,
and substitute CharTy in that case.
Jordan
On Oct 4, 2013, at 5:28 , Anders Rönnholm <[email protected]> wrote:
> Hi,
>
> A new iteration on the checker. I have moved it to be a warning in semaexpr
> now.
>
> //Anders
>
> .......................................................................................................................
> Anders Rönnholm Senior Engineer
> Evidente ES East AB Warfvinges väg 34 SE-112 51 Stockholm Sweden
>
> Mobile: +46 (0)70 912 42 54
> E-mail: [email protected]
>
> www.evidente.se
>
> ________________________________________
> Från: Jordan Rose [[email protected]]
> Skickat: den 2 oktober 2013 18:12
> Till: Daniel Marjamäki
> Cc: Richard Smith; Anders Rönnholm; cfe commits
> Ämne: Re: [PATCH] [StaticAnalyzer] New checker StringPlusChar
>
> On Oct 1, 2013, at 22:16 , Daniel Marjamäki <[email protected]>
> wrote:
>
>>
>> Hello!
>>
>>> This should be a warning, not a static analyser check, shouldn't it?
>>
>> Is anybody against this?
>>
>> Personally I feel very confident about this check regarding signal/noise.
>> However putting it in the static analyser to start with felt like a safe
>> choice.
>
> I'm fine with this. I didn't realize at first it was just for character
> literals; with that restriction this does seem like false positives would be
> unlikely. Thanks, Richard. (Sorry to force another iteration, Anders. The
> existing warning is in SemaExpr.cpp: diagnoseStringPlusInt.)
>
> Daniel, delaying += and char plus string seem fine to me, though they might
> just be handled if you hook into diagnoseStringPlusInt. And thanks for the
> reminder about things like s8, u8, etc.
>
> Jordan
Index: lib/Sema/SemaExpr.cpp
===================================================================
--- lib/Sema/SemaExpr.cpp (revision 191941)
+++ lib/Sema/SemaExpr.cpp (working copy)
@@ -6913,6 +6913,47 @@
Self.Diag(OpLoc, diag::note_string_plus_int_silence);
}
+/// \brief Emit a warning when adding a char literal to a string.
+static void diagnoseStringPlusChar(Sema &Self, SourceLocation OpLoc,
+ Expr *LHSExpr, Expr *RHSExpr) {
+ const DeclRefExpr *StringRefExpr =
+ dyn_cast<DeclRefExpr>(LHSExpr->IgnoreImpCasts());
+ const CharacterLiteral *CharExpr =
+ dyn_cast<CharacterLiteral>(RHSExpr->IgnoreImpCasts());
+
+ if (!StringRefExpr) {
+ StringRefExpr = dyn_cast<DeclRefExpr>(RHSExpr->IgnoreImpCasts());
+ CharExpr = dyn_cast<CharacterLiteral>(LHSExpr->IgnoreImpCasts());
+ }
+
+ if (!CharExpr || !StringRefExpr)
+ return;
+
+ const QualType StringType = StringRefExpr->getType();
+
+ // Return if not a PointerType.
+ if (!StringType->isAnyPointerType())
+ return;
+
+ // Return if not a CharacterType.
+ if (!StringType->getPointeeType()->isAnyCharacterType())
+ return;
+
+ SourceRange DiagRange(LHSExpr->getLocStart(), RHSExpr->getLocEnd());
+ Self.Diag(OpLoc, diag::warn_string_plus_char) << DiagRange;
+
+ // Only print a fixit for str + char, not for char + str.
+ if (isa<CharacterLiteral>(RHSExpr->IgnoreImpCasts())) {
+ SourceLocation EndLoc = Self.PP.getLocForEndOfToken(RHSExpr->getLocEnd());
+ Self.Diag(OpLoc, diag::note_string_plus_char_silence)
+ << FixItHint::CreateInsertion(LHSExpr->getLocStart(), "&")
+ << FixItHint::CreateReplacement(SourceRange(OpLoc), "[")
+ << FixItHint::CreateInsertion(EndLoc, "]");
+ } else {
+ Self.Diag(OpLoc, diag::note_string_plus_char_silence);
+ }
+}
+
/// \brief Emit error when two pointers are incompatible.
static void diagnosePointerIncompatibility(Sema &S, SourceLocation Loc,
Expr *LHSExpr, Expr *RHSExpr) {
@@ -6939,9 +6980,11 @@
if (LHS.isInvalid() || RHS.isInvalid())
return QualType();
- // Diagnose "string literal" '+' int.
- if (Opc == BO_Add)
+ // Diagnose "string literal" '+' int and string '+' "char literal".
+ if (Opc == BO_Add) {
diagnoseStringPlusInt(*this, Loc, LHS.get(), RHS.get());
+ diagnoseStringPlusChar(*this, Loc, LHS.get(), RHS.get());
+ }
// handle the common case first (both operands are arithmetic).
if (!compType.isNull() && compType->isArithmeticType()) {
Index: test/SemaCXX/string-plus-char.cpp
===================================================================
--- test/SemaCXX/string-plus-char.cpp (revision 0)
+++ test/SemaCXX/string-plus-char.cpp (working copy)
@@ -0,0 +1,31 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+class A {
+public:
+ A(): str() { }
+ A(const char *p) { }
+ A(char *p) : str(p + 'a') { } // expected-warning {{adding 'char' to a string pointer does not append to the string}} expected-note {{use array indexing to silence this warning}}
+ A& operator+(const char *p) { return *this; }
+ A& operator+(char ch) { return *this; }
+ char * str;
+};
+
+void f(const char *s) {
+ A a = s + 'a'; // // expected-warning {{adding 'char' to a string pointer does not append to the string}} expected-note {{use array indexing to silence this warning}}
+ a = a + s + 'b'; // no-warning
+
+ char *str = 0;
+ char *str2 = str + 'c'; // expected-warning {{adding 'char' to a string pointer does not append to the string}} expected-note {{use array indexing to silence this warning}}
+
+ const char *constStr = s + 'c'; // expected-warning {{adding 'char' to a string pointer does not append to the string}} expected-note {{use array indexing to silence this warning}}
+
+ str = 'c' + str;// expected-warning {{adding 'char' to a string pointer does not append to the string}} expected-note {{use array indexing to silence this warning}}
+
+ wchar_t *wstr;
+ wstr = wstr + L'c'; // expected-warning {{adding 'char' to a string pointer does not append to the string}} expected-note {{use array indexing to silence this warning}}
+
+ // no-warning
+ char c = 'c';
+ str = str + c;
+ str = c + str;
+}
Index: include/clang/Basic/DiagnosticSemaKinds.td
===================================================================
--- include/clang/Basic/DiagnosticSemaKinds.td (revision 191941)
+++ include/clang/Basic/DiagnosticSemaKinds.td (working copy)
@@ -4271,6 +4271,12 @@
def note_string_plus_int_silence : Note<
"use array indexing to silence this warning">;
+def warn_string_plus_char : Warning<
+ "adding 'char' to a string pointer does not append to the string">,
+ InGroup<StringPlusChar>;
+def note_string_plus_char_silence : Note<
+ "use array indexing to silence this warning">;
+
def warn_sizeof_array_param : Warning<
"sizeof on array function parameter will return size of %0 instead of %1">,
InGroup<SizeofArrayArgument>;
Index: include/clang/Basic/DiagnosticGroups.td
===================================================================
--- include/clang/Basic/DiagnosticGroups.td (revision 191941)
+++ include/clang/Basic/DiagnosticGroups.td (working copy)
@@ -267,6 +267,7 @@
def StaticFloatInit : DiagGroup<"static-float-init", [GNUStaticFloatInit]>;
def GNUStatementExpression : DiagGroup<"gnu-statement-expression">;
def StringPlusInt : DiagGroup<"string-plus-int">;
+def StringPlusChar : DiagGroup<"string-plus-char">;
def StrncatSize : DiagGroup<"strncat-size">;
def TautologicalOutOfRangeCompare : DiagGroup<"tautological-constant-out-of-range-compare">;
def TautologicalCompare : DiagGroup<"tautological-compare",
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
