http://reviews.llvm.org/D3976
Files:
include/clang/Basic/DiagnosticSemaKinds.td
include/clang/Sema/Sema.h
lib/Sema/SemaExpr.cpp
lib/Sema/SemaStmt.cpp
test/SemaCXX/warn-comma-operator.cpp
Index: test/SemaCXX/warn-comma-operator.cpp
===================================================================
--- test/SemaCXX/warn-comma-operator.cpp
+++ test/SemaCXX/warn-comma-operator.cpp
@@ -0,0 +1,293 @@
+// 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) {}
+
+ x++, ++x, x++, ++x, x--, --x, ++y;
+ x = 5, x *= 5, x /= 5, x %= 5, x += 5,x -= 5, ++y;
+ x <<= 5, x >>= 5, x &= 5, x |= 5, 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) {}
+
+ x++, ++x, x++, ++x, x--, --x, ++y;
+ x = 5, x *= 5, x /= 5, x %= 5, x += 5,x -= 5, ++y;
+ x <<= 5, x >>= 5, x &= 5, x |= 5, x ^= 5, ++y;
+}
+
+// Test nested comma operators
+void test3() {
+ int x1, x2, x3;
+ int y1, *y2 = 0, y3 = 5;
+ for (x1 = 5, x2 = 4, x3 = 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}:")"
+
+ for ((x = 0), (y = 0); x < y; (x++), (y++)) {}
+ for (x = 0, y = 0; x < y; x++, y++) {}
+}
+
+bool DoStuff();
+class S9 {
+public:
+ bool Advance();
+ bool More();
+};
+
+// Ignore comma operator in for-loop increments.
+void test9() {
+ int x;
+ for (x = 0; x < 10; DoStuff(), ++x) {}
+ for (S9 s; s.More(); s.Advance(), ++x) {}
+}
+
+// Allow cast to void to silence warning
+void test10() {
+ if (return_four(), true) {}
+ // 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 ((void)return_four(), true) {}
+ if (static_cast<void>(return_four()), true) {}
+}
+
+// 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;
+
+// Allow RAII objects in the LHS.
+bool get_status() {
+ return (MutexLock(StatusMutex), Status);
+ return (MutexLock(), Status);
+ return (BuiltinMutex(), Status);
+}
+}
+
+// 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}:")"
+}
+
+void return_void() { return; }
+
+// Ignore when the LHS is a void returning function.
+void test14(int x) {
+ x = (return_void(), x);
+ int y = (return_void(), x);
+ for (; return_void(), x;) {}
+ while (return_void(), x) {}
+ if (return_void(), x) {}
+ do { } while (return_void(), x);
+ return_void(), return_void(), return_four();
+}
+
+// Nested comma operator with fix-its.
+void test15() {
+ 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}:")"
+}
Index: include/clang/Basic/DiagnosticSemaKinds.td
===================================================================
--- include/clang/Basic/DiagnosticSemaKinds.td
+++ include/clang/Basic/DiagnosticSemaKinds.td
@@ -54,6 +54,10 @@
"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">;
Index: include/clang/Sema/Sema.h
===================================================================
--- include/clang/Sema/Sema.h
+++ include/clang/Sema/Sema.h
@@ -3636,6 +3636,8 @@
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,
Index: lib/Sema/SemaExpr.cpp
===================================================================
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -8728,6 +8728,136 @@
? LHSType : LHSType.getUnqualifiedType());
}
+// A whitelist of expressions on the left hand side of a comma operator that
+// will prevent it from emitting an error.
+static bool IgnoreCommaOperand(const Expr *E) {
+ E = E->IgnoreParens();
+
+ // Allow casts to void to silence the warning.
+ if (const CastExpr *CE = dyn_cast<CastExpr>(E)) {
+ if (CE->getCastKind() == CK_ToVoid) {
+ return true;
+ }
+ }
+
+ // Allow the creation of temporaries that has a cleanup after the RHS is
+ // evaluated. This effectively wraps the RHS between the LHS construction
+ // and destruction, such as during RAII use.
+ if (isa<CXXBindTemporaryExpr>(E->IgnoreCasts())) {
+ return true;
+ }
+
+ // Allow assignment and compound assignments.
+ if (const BinaryOperator *BO = dyn_cast<BinaryOperator>(E)) {
+ switch (BO->getOpcode()) {
+ case BO_Assign:
+ case BO_MulAssign:
+ case BO_DivAssign:
+ case BO_RemAssign:
+ case BO_AddAssign:
+ case BO_SubAssign:
+ case BO_ShlAssign:
+ case BO_ShrAssign:
+ case BO_AndAssign:
+ case BO_XorAssign:
+ case BO_OrAssign:
+ return true;
+ default:
+ return false;
+ }
+ }
+
+ // Increments and decrements are commonly chained together into one line.
+ if (const UnaryOperator *UO = dyn_cast<UnaryOperator>(E)) {
+ switch (UO->getOpcode()) {
+ case UO_PostInc:
+ case UO_PostDec:
+ case UO_PreInc:
+ case UO_PreDec:
+ return true;
+ default:
+ return false;
+ }
+ }
+
+ if (const CallExpr *CE = dyn_cast<CallExpr>(E)) {
+ if (const FunctionDecl *FD = CE->getDirectCallee()) {
+ // Unlikely to have a void value on the left hand side of other operators.
+ if (FD->getReturnType()->isVoidType()) {
+ return true;
+ }
+
+ // These are the overloaded versions of the operators from UnaryOperator
+ // and BinaryOperator above.
+ switch (FD->getOverloadedOperator()) {
+ case OO_PlusPlus:
+ case OO_MinusMinus:
+ case OO_Equal:
+ case OO_PlusEqual:
+ case OO_MinusEqual:
+ case OO_StarEqual:
+ case OO_SlashEqual:
+ case OO_PercentEqual:
+ case OO_CaretEqual:
+ case OO_AmpEqual:
+ case OO_PipeEqual:
+ case OO_LessLessEqual:
+ case OO_GreaterGreaterEqual:
+ return true;
+ default:
+ return false;
+ }
+ }
+ }
+
+ 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 skip only the increment in for
+ // statements, so this check skips the while condition, for-loop condition,
+ // and for-loop increment. In SemaStmt, we call back to here to handle the
+ // two conditions that were skipped.
+ const unsigned ControlFlags =
+ Scope::ControlScope | Scope::ContinueScope | Scope::BreakScope;
+ const unsigned ScopeFlags = getCurScope()->getFlags();
+ if ((ScopeFlags & ControlFlags) == ControlFlags)
+ 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();
+ }
+
+ // Whitelist some expressions to assume to be safe for the left hand
+ // expression
+ 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) {
@@ -8757,6 +8887,9 @@
diag::err_incomplete_type);
}
+ if (!S.getDiagnostics().isIgnored(diag::warn_comma_operator, Loc))
+ S.DiagnoseCommaOperator(LHS.get(), Loc);
+
return RHS.get()->getType();
}
Index: lib/Sema/SemaStmt.cpp
===================================================================
--- lib/Sema/SemaStmt.cpp
+++ lib/Sema/SemaStmt.cpp
@@ -1207,6 +1207,20 @@
}
}
+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::ActOnWhileStmt(SourceLocation WhileLoc, FullExprArg Cond,
Decl *CondVar, Stmt *Body) {
@@ -1224,6 +1238,10 @@
return StmtError();
CheckBreakContinueBinding(ConditionExpr);
+ if (ConditionExpr &&
+ !Diags.isIgnored(diag::warn_comma_operator, ConditionExpr->getExprLoc()))
+ CommaVisitor(*this).Visit(ConditionExpr);
+
DiagnoseUnusedExprResult(Body);
if (isa<NullStmt>(Body))
@@ -1625,6 +1643,11 @@
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);
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
