From f371380fc96c67cab15067dea51a55131757d351 Mon Sep 17 00:00:00 2001 From: Richard Trieu Date: Thu, 25 Oct 2018 01:08:00 +0000 Subject: [PATCH] [Sema] Fix -Wcomma for C89 There is a small difference in the scope flags for C89 versus the other C/C++ dialects. This change ensures that the -Wcomma warning won't be duplicated or issued in the wrong location. Also, the test case is refactored into C and C++ parts, with the C++ parts guarded by a #ifdef to allow the test to run in both modes. https://bugs.llvm.org/show_bug.cgi?id=32370 llvm-svn: 345228 --- clang/lib/Sema/SemaExpr.cpp | 5 +- clang/lib/Sema/SemaStmt.cpp | 10 +- clang/test/SemaCXX/warn-comma-operator.cpp | 243 +++++++++++++++-------------- 3 files changed, 139 insertions(+), 119 deletions(-) diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp index 98025ca..66194c6 100644 --- a/clang/lib/Sema/SemaExpr.cpp +++ b/clang/lib/Sema/SemaExpr.cpp @@ -11309,8 +11309,11 @@ void Sema::DiagnoseCommaOperator(const Expr *LHS, SourceLocation Loc) { // 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. + // Differences in scope flags for C89 mode requires the extra logic. const unsigned ForIncrementFlags = - Scope::ControlScope | Scope::ContinueScope | Scope::BreakScope; + getLangOpts().C99 || getLangOpts().CPlusPlus + ? Scope::ControlScope | Scope::ContinueScope | Scope::BreakScope + : Scope::ContinueScope | Scope::BreakScope; const unsigned ForInitFlags = Scope::ControlScope | Scope::DeclScope; const unsigned ScopeFlags = getCurScope()->getFlags(); if ((ScopeFlags & ForIncrementFlags) == ForIncrementFlags || diff --git a/clang/lib/Sema/SemaStmt.cpp b/clang/lib/Sema/SemaStmt.cpp index 16ed9ed..7fc89bf 100644 --- a/clang/lib/Sema/SemaStmt.cpp +++ b/clang/lib/Sema/SemaStmt.cpp @@ -551,8 +551,9 @@ Sema::ActOnIfStmt(SourceLocation IfLoc, bool IsConstexpr, Stmt *InitStmt, false); Expr *CondExpr = Cond.get().second; - if (!Diags.isIgnored(diag::warn_comma_operator, - CondExpr->getExprLoc())) + // Only call the CommaVisitor when not C89 due to differences in scope flags. + if ((getLangOpts().C99 || getLangOpts().CPlusPlus) && + !Diags.isIgnored(diag::warn_comma_operator, CondExpr->getExprLoc())) CommaVisitor(*this).Visit(CondExpr); if (!elseStmt) @@ -1328,6 +1329,11 @@ Sema::ActOnDoStmt(SourceLocation DoLoc, Stmt *Body, return StmtError(); Cond = CondResult.get(); + // Only call the CommaVisitor for C89 due to differences in scope flags. + if (Cond && !getLangOpts().C99 && !getLangOpts().CPlusPlus && + !Diags.isIgnored(diag::warn_comma_operator, Cond->getExprLoc())) + CommaVisitor(*this).Visit(Cond); + DiagnoseUnusedExprResult(Body); return new (Context) DoStmt(Body, Cond, DoLoc, WhileLoc, CondRParen); diff --git a/clang/test/SemaCXX/warn-comma-operator.cpp b/clang/test/SemaCXX/warn-comma-operator.cpp index 75ef452..0ed127b 100644 --- a/clang/test/SemaCXX/warn-comma-operator.cpp +++ b/clang/test/SemaCXX/warn-comma-operator.cpp @@ -1,8 +1,16 @@ // 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 +// RUN: %clang_cc1 -fsyntax-only -Wcomma -x c -std=c89 -verify %s +// RUN: %clang_cc1 -fsyntax-only -Wcomma -x c -std=c99 -verify %s +// RUN: %clang_cc1 -fsyntax-only -Wcomma -x c -std=c11 -verify %s +// RUN: %clang_cc1 -fsyntax-only -Wcomma -x c -std=c17 -verify %s + +// int returning function +int return_four() { return 5; } + // Test builtin operators -void test1() { +void test_builtin() { int x = 0, y = 0; for (; y < 10; x++, y++) {} for (; y < 10; ++x, y++) {} @@ -23,6 +31,116 @@ void test1() { for (; y < 10; x ^= 5, ++y) {} } +// Test nested comma operators +void test_nested() { + int x1, x2, x3; + int y1, *y2 = 0, y3 = 5; + +#if __STDC_VERSION >= 199901L + for (int z1 = 5, z2 = 4, z3 = 3; x1 <4; ++x1) {} +#endif +} + +// Confusing "," for "==" +void test_compare() { + 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 test_plus() { + 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(); +} + +// Be sure to look through parentheses +void test_parentheses() { + 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}:")" +} + +void test_increment() { + 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(" + // CHECK: fix-it:{{.*}}:{[[@LINE-4]]:6-[[@LINE-4]]:6}:")" +} + +// Check for comma operator in conditions. +void test_conditions(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(" + // 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(" + // 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(" + // 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(" + // 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(" + // 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(" + // CHECK: fix-it:{{.*}}:{[[@LINE-4]]:30-[[@LINE-4]]:30}:")" +} + +// Nested comma operator with fix-its. +void test_nested_fixits() { + 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(" + // CHECK: fix-it:{{.*}}:{[[@LINE-4]]:16-[[@LINE-4]]:16}:")" + // CHECK: fix-it:{{.*}}:{[[@LINE-5]]:18-[[@LINE-5]]:18}:"static_cast(" + // CHECK: fix-it:{{.*}}:{[[@LINE-6]]:31-[[@LINE-6]]:31}:")" + // CHECK: fix-it:{{.*}}:{[[@LINE-7]]:33-[[@LINE-7]]:33}:"static_cast(" + // CHECK: fix-it:{{.*}}:{[[@LINE-8]]:46-[[@LINE-8]]:46}:")" +} + +#ifdef __cplusplus class S2 { public: void advance(); @@ -45,7 +163,7 @@ public: }; // Test overloaded operators -void test2() { +void test_overloaded_operator() { S2 x; int y; for (; y < 10; x++, y++) {} @@ -67,22 +185,13 @@ void test2() { 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() { +void test_stream() { cout << 5 << return_four(); cout << 5, return_four(); // expected-warning@-1{{comma operator}} @@ -91,33 +200,11 @@ void test4() { // 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() { +void test_overloaded_function() { Concat((return_four() , 5)); // expected-warning@-1{{comma operator}} // expected-note@-2{{cast expression to void}} @@ -127,22 +214,6 @@ void test7() { 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}:")" -} - bool DoStuff(); class S9 { public: @@ -151,24 +222,15 @@ public: }; // Ignore comma operator in for-loop initializations and increments. -void test9() { +void test_for_loop() { 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(" - // CHECK: fix-it:{{.*}}:{[[@LINE-4]]:6-[[@LINE-4]]:6}:")" -} - // Ignore comma operator in templates. -namespace test11 { +namespace test_template { template struct B { static const bool value = T; }; @@ -188,7 +250,7 @@ class Foo { const auto X = Foo(); } -namespace test12 { +namespace test_mutex { class Mutex { public: Mutex(); @@ -225,64 +287,13 @@ bool get_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(" - // 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(" - // 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(" - // 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(" - // 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(" - // 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(" - // 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(" - // CHECK: fix-it:{{.*}}:{[[@LINE-4]]:16-[[@LINE-4]]:16}:")" - // CHECK: fix-it:{{.*}}:{[[@LINE-5]]:18-[[@LINE-5]]:18}:"static_cast(" - // CHECK: fix-it:{{.*}}:{[[@LINE-6]]:31-[[@LINE-6]]:31}:")" - // CHECK: fix-it:{{.*}}:{[[@LINE-7]]:33-[[@LINE-7]]:33}:"static_cast(" - // CHECK: fix-it:{{.*}}:{[[@LINE-8]]:46-[[@LINE-8]]:46}:")" -} - // PR39375 - test cast to void to silence warnings template -void test15() { +void test_dependent_cast() { (void)42, 0; static_cast(42), 0; (void)T{}, 0; static_cast(T{}), 0; } +#endif // ifdef __cplusplus -- 2.7.4