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, DefaultIgnore; +def warn_comma_operator : Warning< + "possible misuse of comma operator here">, + InGroup>, DefaultIgnore; + // Constant expressions def err_expr_not_ice : Error< "expression is not an %select{integer|integral}0 constant expression">; Index: lib/Sema/SemaExpr.cpp =================================================================== --- lib/Sema/SemaExpr.cpp +++ lib/Sema/SemaExpr.cpp @@ -8595,6 +8595,102 @@ ? 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(E)) { + if (CE->getCastKind() == CK_ToVoid) { + return true; + } + } + + // Allow assignemtn and compound assignments. Also recurse on other + // comma operators. + if (const BinaryOperator *BO = dyn_cast(E)) { + switch (BO->getOpcode()) { + case BO_Comma: + return IgnoreCommaOperand(BO->getLHS()) && + IgnoreCommaOperand(BO->getRHS()); + 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 widely used in for loops. + if (const UnaryOperator *UO = dyn_cast(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(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. +static void DiagnoseCommaOperator(Sema &S, const Expr *LHS, + SourceLocation Loc) { + if (Loc.isMacroID()) + return; + + if (IgnoreCommaOperand(LHS)) + return; + + S.Diag(Loc, diag::warn_comma_operator); +} + // C99 6.5.17 static QualType CheckCommaOperands(Sema &S, ExprResult &LHS, ExprResult &RHS, SourceLocation Loc) { @@ -8624,6 +8720,8 @@ diag::err_incomplete_type); } + DiagnoseCommaOperator(S, LHS.get(), Loc); + return RHS.get()->getType(); } Index: test/SemaCXX/warn-comma-operator.cpp =================================================================== --- test/SemaCXX/warn-comma-operator.cpp +++ test/SemaCXX/warn-comma-operator.cpp @@ -0,0 +1,138 @@ +// RUN: %clang_cc1 -fsyntax-only -Wcomma -verify %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 (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{{comma operator}} +} + +// Confusing "," for "==" +void test5() { + if (return_four(), 5) {} // expected-warning{{comma operator}} + if (return_four() == 5) {} +} + +// Confusing "," for "+" +int test6() { + return return_four(), return_four(); // expected-warning{{comma operator}} + 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{{comma operator}} + Concat(return_four() , 5); +} + +// Be sure to look through parentheses +void test8() { + int x, y; + for (x = 0; return_four(), x;) {} // expected-warning{{comma operator}} + for (x = 0; (return_four()), (x) ;) {} // expected-warning{{comma operator}} + + for ((x = 0), (y = 0); x < y; (x++), (y++)) {} + for (x = 0, y = 0; x < y; x++, y++) {} +} + +void DoStuff(); +class S9 { +public: + void Advance(); + bool More(); +}; +// Ignore functions with void return type. +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{{comma operator}} + if ((void)return_four(), true) {} + if (static_cast(return_four()), true) {} +}