From: Nico Weber Date: Fri, 2 Mar 2012 22:01:22 +0000 (+0000) Subject: Add -Wstring-plus-int, which warns on "str" + int and int + "str". X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=1cb2d742eb6635aeab6132ee5f0b5781d39487d7;p=clang Add -Wstring-plus-int, which warns on "str" + int and int + "str". It doesn't warn if the integer is known at compile time and within the bounds of the string. Discussion: http://comments.gmane.org/gmane.comp.compilers.clang.scm/47203 git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@151943 91177308-0d34-0410-b5e6-96231b3b80d8 --- diff --git a/include/clang/Basic/DiagnosticGroups.td b/include/clang/Basic/DiagnosticGroups.td index 086a8b54aa..cb59cbad4f 100644 --- a/include/clang/Basic/DiagnosticGroups.td +++ b/include/clang/Basic/DiagnosticGroups.td @@ -161,6 +161,7 @@ def : DiagGroup<"stack-protector">; def : DiagGroup<"switch-default">; def : DiagGroup<"synth">; def SizeofArrayArgument : DiagGroup<"sizeof-array-argument">; +def StringPlusInt : DiagGroup<"string-plus-int">; def StrncatSize : DiagGroup<"strncat-size">; def TautologicalCompare : DiagGroup<"tautological-compare">; def HeaderHygiene : DiagGroup<"header-hygiene">; @@ -326,6 +327,7 @@ def Most : DiagGroup<"most", [ ReturnType, SelfAssignment, SizeofArrayArgument, + StringPlusInt, Trigraphs, Uninitialized, UnknownPragmas, diff --git a/include/clang/Basic/DiagnosticSemaKinds.td b/include/clang/Basic/DiagnosticSemaKinds.td index dc888150b0..1c749e3eb7 100644 --- a/include/clang/Basic/DiagnosticSemaKinds.td +++ b/include/clang/Basic/DiagnosticSemaKinds.td @@ -3430,6 +3430,12 @@ def warn_self_assignment : Warning< "explicitly assigning a variable of type %0 to itself">, InGroup, DefaultIgnore; +def warn_string_plus_int : Warning< + "adding %0 to a string does not append to the string">, + InGroup; +def note_string_plus_int_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; diff --git a/include/clang/Sema/Sema.h b/include/clang/Sema/Sema.h index 7d9f835000..6289b9f3a1 100644 --- a/include/clang/Sema/Sema.h +++ b/include/clang/Sema/Sema.h @@ -6097,7 +6097,7 @@ public: ExprResult &LHS, ExprResult &RHS, SourceLocation Loc, bool IsCompAssign = false); QualType CheckAdditionOperands( // C99 6.5.6 - ExprResult &LHS, ExprResult &RHS, SourceLocation Loc, + ExprResult &LHS, ExprResult &RHS, SourceLocation Loc, unsigned Opc, QualType* CompLHSTy = 0); QualType CheckSubtractionOperands( // C99 6.5.6 ExprResult &LHS, ExprResult &RHS, SourceLocation Loc, diff --git a/lib/Sema/SemaExpr.cpp b/lib/Sema/SemaExpr.cpp index 1bab239ab3..ab96f7416f 100644 --- a/lib/Sema/SemaExpr.cpp +++ b/lib/Sema/SemaExpr.cpp @@ -5948,6 +5948,46 @@ static bool checkArithmethicPointerOnNonFragileABI(Sema &S, return false; } +/// diagnoseStringPlusInt - Emit a warning when adding an integer to a string +/// literal. +static void diagnoseStringPlusInt(Sema &Self, SourceLocation OpLoc, + Expr *LHSExpr, Expr *RHSExpr) { + StringLiteral* StrExpr = dyn_cast(LHSExpr->IgnoreImpCasts()); + Expr* IndexExpr = RHSExpr; + if (!StrExpr) { + StrExpr = dyn_cast(RHSExpr->IgnoreImpCasts()); + IndexExpr = LHSExpr; + } + + bool IsStringPlusInt = StrExpr && + IndexExpr->getType()->isIntegralOrUnscopedEnumerationType(); + if (!IsStringPlusInt) + return; + + llvm::APSInt index; + if (IndexExpr->EvaluateAsInt(index, Self.getASTContext())) { + unsigned StrLenWithNull = StrExpr->getLength() + 1; + if (index.isNonNegative() && + index <= llvm::APSInt(llvm::APInt(index.getBitWidth(), StrLenWithNull), + index.isUnsigned())) + return; + } + + SourceRange DiagRange(LHSExpr->getLocStart(), RHSExpr->getLocEnd()); + Self.Diag(OpLoc, diag::warn_string_plus_int) + << DiagRange << IndexExpr->IgnoreImpCasts()->getType(); + + // Only print a fixit for "str" + int, not for int + "str". + if (IndexExpr == RHSExpr) { + SourceLocation EndLoc = Self.PP.getLocForEndOfToken(RHSExpr->getLocEnd()); + Self.Diag(OpLoc, diag::note_string_plus_int_silence) + << FixItHint::CreateInsertion(LHSExpr->getLocStart(), "&") + << FixItHint::CreateReplacement(SourceRange(OpLoc), "[") + << FixItHint::CreateInsertion(EndLoc, "]"); + } else + Self.Diag(OpLoc, diag::note_string_plus_int_silence); +} + /// \brief Emit error when two pointers are incompatible. static void diagnosePointerIncompatibility(Sema &S, SourceLocation Loc, Expr *LHSExpr, Expr *RHSExpr) { @@ -5959,7 +5999,8 @@ static void diagnosePointerIncompatibility(Sema &S, SourceLocation Loc, } QualType Sema::CheckAdditionOperands( // C99 6.5.6 - ExprResult &LHS, ExprResult &RHS, SourceLocation Loc, QualType* CompLHSTy) { + ExprResult &LHS, ExprResult &RHS, SourceLocation Loc, unsigned Opc, + QualType* CompLHSTy) { checkArithmeticNull(*this, LHS, RHS, Loc, /*isCompare=*/false); if (LHS.get()->getType()->isVectorType() || @@ -5973,6 +6014,10 @@ QualType Sema::CheckAdditionOperands( // C99 6.5.6 if (LHS.isInvalid() || RHS.isInvalid()) return QualType(); + // Diagnose "string literal" '+' int. + if (Opc == BO_Add) + diagnoseStringPlusInt(*this, Loc, LHS.get(), RHS.get()); + // handle the common case first (both operands are arithmetic). if (LHS.get()->getType()->isArithmeticType() && RHS.get()->getType()->isArithmeticType()) { @@ -7669,7 +7714,7 @@ ExprResult Sema::CreateBuiltinBinOp(SourceLocation OpLoc, ResultTy = CheckRemainderOperands(LHS, RHS, OpLoc); break; case BO_Add: - ResultTy = CheckAdditionOperands(LHS, RHS, OpLoc); + ResultTy = CheckAdditionOperands(LHS, RHS, OpLoc, Opc); break; case BO_Sub: ResultTy = CheckSubtractionOperands(LHS, RHS, OpLoc); @@ -7712,7 +7757,7 @@ ExprResult Sema::CreateBuiltinBinOp(SourceLocation OpLoc, ResultTy = CheckAssignmentOperands(LHS.get(), RHS, OpLoc, CompResultTy); break; case BO_AddAssign: - CompResultTy = CheckAdditionOperands(LHS, RHS, OpLoc, &CompLHSTy); + CompResultTy = CheckAdditionOperands(LHS, RHS, OpLoc, Opc, &CompLHSTy); if (!CompResultTy.isNull() && !LHS.isInvalid() && !RHS.isInvalid()) ResultTy = CheckAssignmentOperands(LHS.get(), RHS, OpLoc, CompResultTy); break; diff --git a/test/Sema/builtins.c b/test/Sema/builtins.c index 17888b0c45..b8b03677fd 100644 --- a/test/Sema/builtins.c +++ b/test/Sema/builtins.c @@ -1,4 +1,4 @@ -// RUN: %clang_cc1 %s -fsyntax-only -verify -pedantic -triple=i686-apple-darwin9 +// RUN: %clang_cc1 %s -fsyntax-only -verify -pedantic -Wno-string-plus-int -triple=i686-apple-darwin9 // This test needs to set the target because it uses __builtin_ia32_vec_ext_v4si int test1(float a, int b) { diff --git a/test/SemaCXX/array-bounds-ptr-arith.cpp b/test/SemaCXX/array-bounds-ptr-arith.cpp index ce1ace6f2f..16e2567c53 100644 --- a/test/SemaCXX/array-bounds-ptr-arith.cpp +++ b/test/SemaCXX/array-bounds-ptr-arith.cpp @@ -1,4 +1,4 @@ -// RUN: %clang_cc1 -verify -Warray-bounds-pointer-arithmetic %s +// RUN: %clang_cc1 -verify -Wno-string-plus-int -Warray-bounds-pointer-arithmetic %s void swallow (const char *x) { (void)x; } void test_pointer_arithmetic(int n) { diff --git a/test/SemaCXX/constant-expression-cxx11.cpp b/test/SemaCXX/constant-expression-cxx11.cpp index dcc437aed0..66fd064fed 100644 --- a/test/SemaCXX/constant-expression-cxx11.cpp +++ b/test/SemaCXX/constant-expression-cxx11.cpp @@ -1,4 +1,4 @@ -// RUN: %clang_cc1 -triple i686-linux -fsyntax-only -verify -std=c++11 -pedantic %s -Wno-comment +// RUN: %clang_cc1 -triple i686-linux -Wno-string-plus-int -fsyntax-only -verify -std=c++11 -pedantic %s -Wno-comment namespace StaticAssertFoldTest { diff --git a/test/SemaCXX/null_in_arithmetic_ops.cpp b/test/SemaCXX/null_in_arithmetic_ops.cpp index 24590ce633..a6c0dbfc65 100644 --- a/test/SemaCXX/null_in_arithmetic_ops.cpp +++ b/test/SemaCXX/null_in_arithmetic_ops.cpp @@ -1,4 +1,4 @@ -// RUN: %clang_cc1 -triple x86_64-unknown-unknown -fsyntax-only -fblocks -Wnull-arithmetic -verify %s +// RUN: %clang_cc1 -triple x86_64-unknown-unknown -fsyntax-only -fblocks -Wnull-arithmetic -verify -Wno-string-plus-int %s #include void f() { diff --git a/test/SemaCXX/string-plus-int.cpp b/test/SemaCXX/string-plus-int.cpp new file mode 100644 index 0000000000..3be3f07033 --- /dev/null +++ b/test/SemaCXX/string-plus-int.cpp @@ -0,0 +1,62 @@ +// RUN: %clang_cc1 -fsyntax-only -verify -Wno-array-bounds %s -fpascal-strings + +void consume(const char* c) {} +void consume(const unsigned char* c) {} +void consume(const wchar_t* c) {} +void consumeChar(char c) {} + +enum MyEnum { + kMySmallEnum = 1, + kMyEnum = 5 +}; + +enum OperatorOverloadEnum { + kMyOperatorOverloadedEnum = 5 +}; + +const char* operator+(const char* c, OperatorOverloadEnum e) { + return "yo"; +} + +const char* operator+(OperatorOverloadEnum e, const char* c) { + return "yo"; +} + +void f(int index) { + // Should warn. + consume("foo" + 5); // expected-warning {{adding 'int' to a string does not append to the string}} expected-note {{use array indexing to silence this warning}} + consume("foo" + index); // expected-warning {{adding 'int' to a string does not append to the string}} expected-note {{use array indexing to silence this warning}} + consume("foo" + kMyEnum); // expected-warning {{adding 'MyEnum' to a string does not append to the string}} expected-note {{use array indexing to silence this warning}} + + consume(5 + "foo"); // expected-warning {{adding 'int' to a string does not append to the string}} expected-note {{use array indexing to silence this warning}} + consume(index + "foo"); // expected-warning {{adding 'int' to a string does not append to the string}} expected-note {{use array indexing to silence this warning}} + consume(kMyEnum + "foo"); // expected-warning {{adding 'MyEnum' to a string does not append to the string}} expected-note {{use array indexing to silence this warning}} + + // FIXME: suggest replacing with "foo"[5] + consumeChar(*("foo" + 5)); // expected-warning {{adding 'int' to a string does not append to the string}} expected-note {{use array indexing to silence this warning}} + consumeChar(*(5 + "foo")); // expected-warning {{adding 'int' to a string does not append to the string}} expected-note {{use array indexing to silence this warning}} + + consume(L"foo" + 5); // expected-warning {{adding 'int' to a string does not append to the string}} expected-note {{use array indexing to silence this warning}} + + // Should not warn. + consume(&("foo"[3])); + consume(&("foo"[index])); + consume(&("foo"[kMyEnum])); + consume("foo" + kMySmallEnum); + consume(kMySmallEnum + "foo"); + + consume(L"foo" + 2); + + consume("foo" + 3); // Points at the \0 + consume("foo" + 4); // Points 1 past the \0, which is legal too. + consume("\pfoo" + 4); // Pascal strings don't have a trailing \0, but they + // have a leading length byte, so this is fine too. + + consume("foo" + kMyOperatorOverloadedEnum); + consume(kMyOperatorOverloadedEnum + "foo"); + + #define A "foo" + #define B "bar" + consume(A B + sizeof(A) - 1); +} +