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
llvm-svn: 151943

clang/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,

clang/include/clang/Basic/DiagnosticSemaKinds.td

@@ -3430,6 +3430,12 @@ def warn_self_assignment : Warning<
   "explicitly assigning a variable of type %0 to itself">,
   InGroup<SelfAssignment>, DefaultIgnore;
 
+def warn_string_plus_int : Warning<
+  "adding %0 to a string does not append to the string">,
+  InGroup<StringPlusInt>;
+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<SizeofArrayArgument>;

clang/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,

clang/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<StringLiteral>(LHSExpr->IgnoreImpCasts());
+  Expr* IndexExpr = RHSExpr;
+  if (!StrExpr) {
+    StrExpr = dyn_cast<StringLiteral>(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;

clang/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) {

clang/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) {

clang/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 {
 

clang/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 <stddef.h>
 
 void f() {

clang/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);
+}
+