Add -Wcomma warning to Clang.

-Wcomma will detect and warn on most uses of the builtin comma operator.  It
currently whitelists the first and third statements of the for-loop.  For other
cases, the warning can be silenced by casting the first operand of the comma
operator to void.

Differential Revision: http://reviews.llvm.org/D3976

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@261278 91177308-0d34-0410-b5e6-96231b3b80d8

diff --git a/include/clang/Basic/DiagnosticSemaKinds.td b/include/clang/Basic/DiagnosticSemaKinds.td
index 434f06df89916b0519888dba67bd42df2dea21a0..868d02163fae3806f7945fa5f81a063b7051e8d5 100644 (file)
--- a/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/include/clang/Basic/DiagnosticSemaKinds.td
@@ -74,6 +74,10 @@ def warn_infinite_recursive_function : Warning<
   "all paths through this function will call itself">,
   InGroup<InfiniteRecursion>, DefaultIgnore;
 
+def warn_comma_operator : Warning<"possible misuse of comma operator here">,
+  InGroup<DiagGroup<"comma">>, DefaultIgnore;
+def note_cast_to_void : Note<"cast expression to void to silence warning">;
+
 // Constant expressions
 def err_expr_not_ice : Error<
   "expression is not an %select{integer|integral}0 constant expression">;

diff --git a/include/clang/Sema/Sema.h b/include/clang/Sema/Sema.h
index c5b9578b3900073bdb350db035947079d24d3632..ba2e23180ea52e683ebbd132306e5aaa9176d538 100644 (file)
--- a/include/clang/Sema/Sema.h
+++ b/include/clang/Sema/Sema.h
@@ -3971,6 +3971,8 @@ public:
   ExprResult CreateBuiltinBinOp(SourceLocation OpLoc, BinaryOperatorKind Opc,
                                 Expr *LHSExpr, Expr *RHSExpr);
 
+  void DiagnoseCommaOperator(const Expr *LHS, SourceLocation Loc);
+
   /// ActOnConditionalOp - Parse a ?: operation.  Note that 'LHS' may be null
   /// in the case of a the GNU conditional expr extension.
   ExprResult ActOnConditionalOp(SourceLocation QuestionLoc,

diff --git a/lib/Sema/SemaExpr.cpp b/lib/Sema/SemaExpr.cpp
index cbac36b7bffde5873595d22043bfe0a73ad7e455..f3714072ce16985aeff88d2632ccb4361fd6196b 100644 (file)
--- a/lib/Sema/SemaExpr.cpp
+++ b/lib/Sema/SemaExpr.cpp
@@ -9867,6 +9867,67 @@ QualType Sema::CheckAssignmentOperands(Expr *LHSExpr, ExprResult &RHS,
           ? LHSType : LHSType.getUnqualifiedType());
 }
 
+// Only ignore explicit casts to void.
+static bool IgnoreCommaOperand(const Expr *E) {
+  E = E->IgnoreParens();
+
+  if (const CastExpr *CE = dyn_cast<CastExpr>(E)) {
+    if (CE->getCastKind() == CK_ToVoid) {
+      return true;
+    }
+  }
+
+  return false;
+}
+
+// Look for instances where it is likely the comma operator is confused with
+// another operator.  There is a whitelist of acceptable expressions for the
+// left hand side of the comma operator, otherwise emit a warning.
+void Sema::DiagnoseCommaOperator(const Expr *LHS, SourceLocation Loc) {
+  // No warnings in macros
+  if (Loc.isMacroID())
+    return;
+
+  // Don't warn in template instantiations.
+  if (!ActiveTemplateInstantiations.empty())
+    return;
+
+  // Scope isn't fine-grained enough to whitelist the specific cases, so
+  // instead, skip more than needed, then call back into here with the
+  // CommaVisitor in SemaStmt.cpp.
+  // The whitelisted locations are the initialization and increment portions
+  // of a for loop.  The additional checks are on the condition of
+  // if statements, do/while loops, and for loops.
+  const unsigned ForIncreamentFlags =
+      Scope::ControlScope | Scope::ContinueScope | Scope::BreakScope;
+  const unsigned ForInitFlags = Scope::ControlScope | Scope::DeclScope;
+  const unsigned ScopeFlags = getCurScope()->getFlags();
+  if ((ScopeFlags & ForIncrementFlags) == ForIncrementFlags ||
+      (ScopeFlags & ForInitFlags) == ForInitFlags)
+    return;
+
+  // If there are multiple comma operators used together, get the RHS of the
+  // of the comma operator as the LHS.
+  while (const BinaryOperator *BO = dyn_cast<BinaryOperator>(LHS)) {
+    if (BO->getOpcode() != BO_Comma)
+      break;
+    LHS = BO->getRHS();
+  }
+
+  // Only allow some expressions on LHS to not warn.
+  if (IgnoreCommaOperand(LHS))
+    return;
+
+  Diag(Loc, diag::warn_comma_operator);
+  Diag(LHS->getLocStart(), diag::note_cast_to_void)
+      << LHS->getSourceRange()
+      << FixItHint::CreateInsertion(LHS->getLocStart(),
+                                    LangOpts.CPlusPlus ? "static_cast<void>("
+                                                       : "(void)(")
+      << FixItHint::CreateInsertion(PP.getLocForEndOfToken(LHS->getLocEnd()),
+                                    ")");
+}
+
 // C99 6.5.17
 static QualType CheckCommaOperands(Sema &S, ExprResult &LHS, ExprResult &RHS,
                                    SourceLocation Loc) {
@@ -9896,6 +9957,9 @@ static QualType CheckCommaOperands(Sema &S, ExprResult &LHS, ExprResult &RHS,
                             diag::err_incomplete_type);
   }
 
+  if (!S.getDiagnostics().isIgnored(diag::warn_comma_operator, Loc))
+    S.DiagnoseCommaOperator(LHS.get(), Loc);
+
   return RHS.get()->getType();
 }
 

diff --git a/lib/Sema/SemaStmt.cpp b/lib/Sema/SemaStmt.cpp
index 350f16d93d8c4bc4c4ff98a51db1e15f91e13456..c58bf4640049369e0f50d3c4ac9b7dc63445cf04 100644 (file)
--- a/lib/Sema/SemaStmt.cpp
+++ b/lib/Sema/SemaStmt.cpp
@@ -488,6 +488,20 @@ StmtResult Sema::ActOnAttributedStmt(SourceLocation AttrLoc,
   return LS;
 }
 
+namespace {
+class CommaVisitor : public EvaluatedExprVisitor<CommaVisitor> {
+  typedef EvaluatedExprVisitor<CommaVisitor> Inherited;
+  Sema &SemaRef;
+public:
+  CommaVisitor(Sema &SemaRef) : Inherited(SemaRef.Context), SemaRef(SemaRef) {}
+  void VisitBinaryOperator(BinaryOperator *E) {
+    if (E->getOpcode() == BO_Comma)
+      SemaRef.DiagnoseCommaOperator(E->getLHS(), E->getExprLoc());
+    EvaluatedExprVisitor<CommaVisitor>::VisitBinaryOperator(E);
+  }
+};
+}
+
 StmtResult
 Sema::ActOnIfStmt(SourceLocation IfLoc, FullExprArg CondVal, Decl *CondVar,
                   Stmt *thenStmt, SourceLocation ElseLoc,
@@ -502,6 +516,11 @@ Sema::ActOnIfStmt(SourceLocation IfLoc, FullExprArg CondVal, Decl *CondVar,
   }
   Expr *ConditionExpr = CondResult.getAs<Expr>();
   if (ConditionExpr) {
+
+    if (!Diags.isIgnored(diag::warn_comma_operator,
+                         ConditionExpr->getExprLoc()))
+      CommaVisitor(*this).Visit(ConditionExpr);
+
     DiagnoseUnusedExprResult(thenStmt);
 
     if (!elseStmt) {
@@ -1240,6 +1259,10 @@ Sema::ActOnWhileStmt(SourceLocation WhileLoc, FullExprArg Cond,
     return StmtError();
   CheckBreakContinueBinding(ConditionExpr);
 
+  if (ConditionExpr &&
+      !Diags.isIgnored(diag::warn_comma_operator, ConditionExpr->getExprLoc()))
+    CommaVisitor(*this).Visit(ConditionExpr);
+
   DiagnoseUnusedExprResult(Body);
 
   if (isa<NullStmt>(Body))
@@ -1642,6 +1665,11 @@ Sema::ActOnForStmt(SourceLocation ForLoc, SourceLocation LParenLoc,
       return StmtError();
   }
 
+  if (SecondResult.get() &&
+      !Diags.isIgnored(diag::warn_comma_operator,
+                       SecondResult.get()->getExprLoc()))
+    CommaVisitor(*this).Visit(SecondResult.get());
+
   Expr *Third  = third.release().getAs<Expr>();
 
   DiagnoseUnusedExprResult(First);

diff --git a/test/SemaCXX/warn-comma-operator.cpp b/test/SemaCXX/warn-comma-operator.cpp
new file mode 100644 (file)
index 0000000..3192f68
--- /dev/null
+++ b/test/SemaCXX/warn-comma-operator.cpp
@@ -0,0 +1,278 @@
+// RUN: %clang_cc1 -fsyntax-only -Wcomma -std=c++11 -verify %s
+// RUN: %clang_cc1 -fsyntax-only -Wcomma -std=c++11 -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+
+// Test builtin operators
+void test1() {
+  int x = 0, y = 0;
+  for (; y < 10; x++, y++) {}
+  for (; y < 10; ++x, y++) {}
+  for (; y < 10; x++, ++y) {}
+  for (; y < 10; ++x, ++y) {}
+  for (; y < 10; x--, ++y) {}
+  for (; y < 10; --x, ++y) {}
+  for (; y < 10; x = 5, ++y) {}
+  for (; y < 10; x *= 5, ++y) {}
+  for (; y < 10; x /= 5, ++y) {}
+  for (; y < 10; x %= 5, ++y) {}
+  for (; y < 10; x += 5, ++y) {}
+  for (; y < 10; x -= 5, ++y) {}
+  for (; y < 10; x <<= 5, ++y) {}
+  for (; y < 10; x >>= 5, ++y) {}
+  for (; y < 10; x &= 5, ++y) {}
+  for (; y < 10; x |= 5, ++y) {}
+  for (; y < 10; x ^= 5, ++y) {}
+}
+
+class S2 {
+public:
+  void advance();
+
+  S2 operator++();
+  S2 operator++(int);
+  S2 operator--();
+  S2 operator--(int);
+  S2 operator=(int);
+  S2 operator*=(int);
+  S2 operator/=(int);
+  S2 operator%=(int);
+  S2 operator+=(int);
+  S2 operator-=(int);
+  S2 operator<<=(int);
+  S2 operator>>=(int);
+  S2 operator&=(int);
+  S2 operator|=(int);
+  S2 operator^=(int);
+};
+
+// Test overloaded operators
+void test2() {
+  S2 x;
+  int y;
+  for (; y < 10; x++, y++) {}
+  for (; y < 10; ++x, y++) {}
+  for (; y < 10; x++, ++y) {}
+  for (; y < 10; ++x, ++y) {}
+  for (; y < 10; x--, ++y) {}
+  for (; y < 10; --x, ++y) {}
+  for (; y < 10; x = 5, ++y) {}
+  for (; y < 10; x *= 5, ++y) {}
+  for (; y < 10; x /= 5, ++y) {}
+  for (; y < 10; x %= 5, ++y) {}
+  for (; y < 10; x += 5, ++y) {}
+  for (; y < 10; x -= 5, ++y) {}
+  for (; y < 10; x <<= 5, ++y) {}
+  for (; y < 10; x >>= 5, ++y) {}
+  for (; y < 10; x &= 5, ++y) {}
+  for (; y < 10; x |= 5, ++y) {}
+  for (; y < 10; x ^= 5, ++y) {}
+}
+
+// Test nested comma operators
+void test3() {
+  int x1, x2, x3;
+  int y1, *y2 = 0, y3 = 5;
+  for (int z1 = 5, z2 = 4, z3 = 3; x1 <4; ++x1) {}
+}
+
+class Stream {
+ public:
+  Stream& operator<<(int);
+} cout;
+
+int return_four() { return 5; }
+
+// Confusing "," for "<<"
+void test4() {
+  cout << 5 << return_four();
+  cout << 5, return_four();
+  // expected-warning@-1{{comma operator}}
+  // expected-note@-2{{cast expression to void}}
+  // CHECK: fix-it:{{.*}}:{[[@LINE-3]]:3-[[@LINE-3]]:3}:"static_cast<void>("
+  // CHECK: fix-it:{{.*}}:{[[@LINE-4]]:12-[[@LINE-4]]:12}:")"
+}
+
+// Confusing "," for "=="
+void test5() {
+  if (return_four(), 5) {}
+  // expected-warning@-1{{comma operator}}
+  // expected-note@-2{{cast expression to void}}
+  // CHECK: fix-it:{{.*}}:{[[@LINE-3]]:7-[[@LINE-3]]:7}:"static_cast<void>("
+  // CHECK: fix-it:{{.*}}:{[[@LINE-4]]:20-[[@LINE-4]]:20}:")"
+
+  if (return_four() == 5) {}
+}
+
+// Confusing "," for "+"
+int test6() {
+  return return_four(), return_four();
+  // expected-warning@-1{{comma operator}}
+  // expected-note@-2{{cast expression to void}}
+  // CHECK: fix-it:{{.*}}:{[[@LINE-3]]:10-[[@LINE-3]]:10}:"static_cast<void>("
+  // CHECK: fix-it:{{.*}}:{[[@LINE-4]]:23-[[@LINE-4]]:23}:")"
+
+  return return_four() + return_four();
+}
+
+void Concat(int);
+void Concat(int, int);
+
+// Testing extra parentheses in function call
+void test7() {
+  Concat((return_four() , 5));
+  // expected-warning@-1{{comma operator}}
+  // expected-note@-2{{cast expression to void}}
+  // CHECK: fix-it:{{.*}}:{[[@LINE-3]]:11-[[@LINE-3]]:11}:"static_cast<void>("
+  // CHECK: fix-it:{{.*}}:{[[@LINE-4]]:24-[[@LINE-4]]:24}:")"
+
+  Concat(return_four() , 5);
+}
+
+// Be sure to look through parentheses
+void test8() {
+  int x, y;
+  for (x = 0; return_four(), x;) {}
+  // expected-warning@-1{{comma operator}}
+  // expected-note@-2{{cast expression to void}}
+  // CHECK: fix-it:{{.*}}:{[[@LINE-3]]:15-[[@LINE-3]]:15}:"static_cast<void>("
+  // CHECK: fix-it:{{.*}}:{[[@LINE-4]]:28-[[@LINE-4]]:28}:")"
+
+  for (x = 0; (return_four()), (x) ;) {}
+  // expected-warning@-1{{comma operator}}
+  // expected-note@-2{{cast expression to void}}
+  // CHECK: fix-it:{{.*}}:{[[@LINE-3]]:15-[[@LINE-3]]:15}:"static_cast<void>("
+  // CHECK: fix-it:{{.*}}:{[[@LINE-4]]:30-[[@LINE-4]]:30}:")"
+}
+
+bool DoStuff();
+class S9 {
+public:
+ bool Advance();
+ bool More();
+};
+
+// Ignore comma operator in for-loop initializations and increments.
+void test9() {
+  int x, y;
+  for (x = 0, y = 5; x < y; ++x) {}
+  for (x = 0; x < 10; DoStuff(), ++x) {}
+  for (S9 s; s.More(); s.Advance(), ++x) {}
+}
+
+void test10() {
+  int x, y;
+  ++x, ++y;
+  // expected-warning@-1{{comma operator}}
+  // expected-note@-2{{cast expression to void}}
+  // CHECK: fix-it:{{.*}}:{[[@LINE-3]]:3-[[@LINE-3]]:3}:"static_cast<void>("
+  // CHECK: fix-it:{{.*}}:{[[@LINE-4]]:6-[[@LINE-4]]:6}:")"
+}
+
+// Ignore comma operator in templates.
+namespace test11 {
+template <bool T>
+struct B { static const bool value = T; };
+
+typedef B<true> true_type;
+typedef B<false> false_type;
+
+template <bool...>
+struct bool_seq;
+
+template <typename... xs>
+class Foo {
+  typedef bool_seq<(xs::value, true)...> all_true;
+  typedef bool_seq<(xs::value, false)...> all_false;
+  typedef bool_seq<xs::value...> seq;
+};
+
+const auto X = Foo<true_type>();
+}
+
+namespace test12 {
+class Mutex {
+ public:
+  Mutex();
+  ~Mutex();
+};
+class MutexLock {
+public:
+  MutexLock(Mutex &);
+  MutexLock();
+  ~MutexLock();
+};
+class BuiltinMutex {
+  Mutex M;
+};
+Mutex StatusMutex;
+bool Status;
+
+bool get_status() {
+  return (MutexLock(StatusMutex), Status);
+  // expected-warning@-1{{comma operator}}
+  // expected-note@-2{{cast expression to void}}
+  // CHECK: fix-it:{{.*}}:{[[@LINE-3]]:11-[[@LINE-3]]:11}:"static_cast<void>("
+  // CHECK: fix-it:{{.*}}:{[[@LINE-4]]:33-[[@LINE-4]]:33}:")"
+  return (MutexLock(), Status);
+  // expected-warning@-1{{comma operator}}
+  // expected-note@-2{{cast expression to void}}
+  // CHECK: fix-it:{{.*}}:{[[@LINE-3]]:11-[[@LINE-3]]:11}:"static_cast<void>("
+  // CHECK: fix-it:{{.*}}:{[[@LINE-4]]:22-[[@LINE-4]]:22}:")"
+  return (BuiltinMutex(), Status);
+  // expected-warning@-1{{comma operator}}
+  // expected-note@-2{{cast expression to void}}
+  // CHECK: fix-it:{{.*}}:{[[@LINE-3]]:11-[[@LINE-3]]:11}:"static_cast<void>("
+  // CHECK: fix-it:{{.*}}:{[[@LINE-4]]:25-[[@LINE-4]]:25}:")"
+}
+}
+
+// Check for comma operator in conditions.
+void test13(int x) {
+  x = (return_four(), x);
+  // expected-warning@-1{{comma operator}}
+  // expected-note@-2{{cast expression to void}}
+  // CHECK: fix-it:{{.*}}:{[[@LINE-3]]:8-[[@LINE-3]]:8}:"static_cast<void>("
+  // CHECK: fix-it:{{.*}}:{[[@LINE-4]]:21-[[@LINE-4]]:21}:")"
+
+  int y = (return_four(), x);
+  // expected-warning@-1{{comma operator}}
+  // expected-note@-2{{cast expression to void}}
+  // CHECK: fix-it:{{.*}}:{[[@LINE-3]]:12-[[@LINE-3]]:12}:"static_cast<void>("
+  // CHECK: fix-it:{{.*}}:{[[@LINE-4]]:25-[[@LINE-4]]:25}:")"
+
+  for (; return_four(), x;) {}
+  // expected-warning@-1{{comma operator}}
+  // expected-note@-2{{cast expression to void}}
+  // CHECK: fix-it:{{.*}}:{[[@LINE-3]]:10-[[@LINE-3]]:10}:"static_cast<void>("
+  // CHECK: fix-it:{{.*}}:{[[@LINE-4]]:23-[[@LINE-4]]:23}:")"
+
+  while (return_four(), x) {}
+  // expected-warning@-1{{comma operator}}
+  // expected-note@-2{{cast expression to void}}
+  // CHECK: fix-it:{{.*}}:{[[@LINE-3]]:10-[[@LINE-3]]:10}:"static_cast<void>("
+  // CHECK: fix-it:{{.*}}:{[[@LINE-4]]:23-[[@LINE-4]]:23}:")"
+
+  if (return_four(), x) {}
+  // expected-warning@-1{{comma operator}}
+  // expected-note@-2{{cast expression to void}}
+  // CHECK: fix-it:{{.*}}:{[[@LINE-3]]:7-[[@LINE-3]]:7}:"static_cast<void>("
+  // CHECK: fix-it:{{.*}}:{[[@LINE-4]]:20-[[@LINE-4]]:20}:")"
+
+  do { } while (return_four(), x);
+  // expected-warning@-1{{comma operator}}
+  // expected-note@-2{{cast expression to void}}
+  // CHECK: fix-it:{{.*}}:{[[@LINE-3]]:17-[[@LINE-3]]:17}:"static_cast<void>("
+  // CHECK: fix-it:{{.*}}:{[[@LINE-4]]:30-[[@LINE-4]]:30}:")"
+}
+
+// Nested comma operator with fix-its.
+void test14() {
+  return_four(), return_four(), return_four(), return_four();
+  // expected-warning@-1 3{{comma operator}}
+  // expected-note@-2 3{{cast expression to void}}
+  // CHECK: fix-it:{{.*}}:{[[@LINE-3]]:3-[[@LINE-3]]:3}:"static_cast<void>("
+  // CHECK: fix-it:{{.*}}:{[[@LINE-4]]:16-[[@LINE-4]]:16}:")"
+  // CHECK: fix-it:{{.*}}:{[[@LINE-5]]:18-[[@LINE-5]]:18}:"static_cast<void>("
+  // CHECK: fix-it:{{.*}}:{[[@LINE-6]]:31-[[@LINE-6]]:31}:")"
+  // CHECK: fix-it:{{.*}}:{[[@LINE-7]]:33-[[@LINE-7]]:33}:"static_cast<void>("
+  // CHECK: fix-it:{{.*}}:{[[@LINE-8]]:46-[[@LINE-8]]:46}:")"
+}