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; +def note_cast_to_null : 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 @@ -3621,6 +3621,9 @@ 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 @@ -8595,6 +8595,119 @@ ? 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->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. +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; + + // Whitelist some expressions to assume to be safe for the left hand + // expression + if (IgnoreCommaOperand(LHS)) + return; + + const BinaryOperator *BO = nullptr; + while ((BO = dyn_cast(LHS)) && BO->getOpcode() == BO_Comma) { + LHS = BO->getRHS(); + } + + Diag(Loc, diag::warn_comma_operator); + Diag(LHS->getLocStart(), diag::note_cast_to_null) + << LHS->getSourceRange() + << FixItHint::CreateInsertion(LHS->getLocStart(), + LangOpts.CPlusPlus ? "static_cast(" + : "(void)(") + << FixItHint::CreateInsertion(PP.getLocForEndOfToken(LHS->getLocEnd()), + ")"); +} + // C99 6.5.17 static QualType CheckCommaOperands(Sema &S, ExprResult &LHS, ExprResult &RHS, SourceLocation Loc) { @@ -8624,6 +8737,8 @@ diag::err_incomplete_type); } + S.DiagnoseCommaOperator(LHS.get(), Loc); + return RHS.get()->getType(); } Index: lib/Sema/SemaOverload.cpp =================================================================== --- lib/Sema/SemaOverload.cpp +++ lib/Sema/SemaOverload.cpp @@ -10887,10 +10887,14 @@ if (Fns.empty()) { // If there are no functions to store, just build a dependent // BinaryOperator or CompoundAssignment. - if (Opc <= BO_Assign || Opc > BO_OrAssign) + if (Opc <= BO_Assign || Opc > BO_OrAssign) { + if (Opc == BO_Comma) { + DiagnoseCommaOperator(Args[0], OpLoc); + } return new (Context) BinaryOperator( Args[0], Args[1], Opc, Context.DependentTy, VK_RValue, OK_Ordinary, OpLoc, FPFeatures.fp_contract); + } return new (Context) CompoundAssignOperator( Args[0], Args[1], Opc, Context.DependentTy, VK_LValue, OK_Ordinary, Index: test/SemaCXX/warn-comma-operator.cpp =================================================================== --- test/SemaCXX/warn-comma-operator.cpp +++ test/SemaCXX/warn-comma-operator.cpp @@ -0,0 +1,202 @@ +// 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 (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(" + // 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(" + // 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(" + // 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(" + // 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(" + // 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(" + // 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++) {} +} + +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@-1{{comma operator}} + // expected-note@-2{{cast expression to void}} + // CHECK: fix-it:{{.*}}:{[[@LINE-3]]:7-[[@LINE-3]]:7}:"static_cast(" + // CHECK: fix-it:{{.*}}:{[[@LINE-4]]:20-[[@LINE-4]]:20}:")" + + if ((void)return_four(), true) {} + if (static_cast(return_four()), true) {} +} + +namespace test11 { +template +struct B { static const bool value = T; }; + +typedef B true_type; +typedef B false_type; + +template +struct bool_seq; + +template +class Foo { + typedef bool_seq<(xs::value, true)...> all_true; + // expected-warning@-1{{comma operator}} + // expected-note@-2{{cast expression to void}} + // CHECK: fix-it:{{.*}}:{[[@LINE-3]]:21-[[@LINE-3]]:21}:"static_cast(" + // CHECK: fix-it:{{.*}}:{[[@LINE-4]]:30-[[@LINE-4]]:30}:")" + + typedef bool_seq<(xs::value, false)...> all_false; + // expected-warning@-1{{comma operator}} + // expected-note@-2{{cast expression to void}} + // CHECK: fix-it:{{.*}}:{[[@LINE-3]]:21-[[@LINE-3]]:21}:"static_cast(" + // CHECK: fix-it:{{.*}}:{[[@LINE-4]]:30-[[@LINE-4]]:30}:")" + + typedef bool_seq seq; +}; + +const auto X = Foo(); +}