-Wcomma, a new warning for questionable uses of the comma operator

[rtrieu]

Summary

-Wcomma emits a warning when there is a questionable use of the comma operator. It does this by only allowing certain expressions on the LHS of the comma operator, with all other expressions giving a warning. The current whitelisted expressions are increments, decrements, assignments, compound assignments, overloaded versions of these operators, and void returning functions. Some examples of code that will be detected:

int foo();
if (foo(), 5) {} // should be "=="
cout << "foo is " , foo();  // should be "<<"

void bar(int);
void bar(int, int);
bar((foo(), foo()));  // Too many parens, calls the one argument function

[rtrieu]

Add a note to the warning that it can be silenced by casting the left hand expression to void.
Catch a few more cases involving templates.

[rtrieu]

Ping.

[rnk]

How do we feel about -Wcomma? I've heard people complain that GCC's -Wparentheses is not very self-documenting.

include/clang/Sema/Sema.h

3626 ↗

(On Diff #10379)

micro nit: extra blank line

lib/Sema/SemaExpr.cpp

8610 ↗

(On Diff #10379)

typo assignemtn

8615 ↗

(On Diff #10379)

Any reason to only look at the RHS?

8683 ↗

(On Diff #10379)

Can you explain why a bit? I'm assuming the answer is that people use the comma in macros to simulate multiple statements, and they end up passing the result as a function argument.

8687 ↗

(On Diff #10379)

Micro nit: end the sentence with a period.

8687 ↗

(On Diff #10379)

Can you elaborate a tiny bit? I'm guessing the idea here is to diagnose once prior to instantiation, even if there are some things that we can't diagnose without instantiating:

template <typename T>
struct A {
  void foo(int x) {}
  void bar() {
    foo((T::baz(), T::qux()));
  }
};

[rtrieu]

include/clang/Sema/Sema.h

3626 ↗

(On Diff #10379)

Randomly selected one of the two lines and removed it.

lib/Sema/SemaExpr.cpp

8610 ↗

(On Diff #10379)

Unscrambled the letters.

8615 ↗

(On Diff #10379)

`-BinaryOperator : outer
  |-BinaryOperator : inner
  | |-LHS
  | `-RHS
  `-RHS : outer

Both BinaryOperators are comma operators.

Currently, this is processing the the outer BinaryOperator and encounters the inner BinaryOperator. We recurse down the RHS to find which expression will be used. The LHS will be checked when processing the inner BinaryOperator.

8683 ↗

(On Diff #10379)

It is common practice to ignore warnings in macros. One common case is macros which need to return a value, but also perform a logging or checking operation at the same time.

#define CHECK_AND_RETURN_VALUE(x) \
  (log(x), x)

void bar(int num) {
  foo(CHECK_AND_RETURN_VALUE(num));
}

8687 ↗

(On Diff #10379)

Period added.

8687 ↗

(On Diff #10379)

This prevents one warning in each template instantiation. Instead, only one warning will be given on the template definition. For cases that are ambiguous, the warning assumes that type-dependent comma operators are bad and warns on them.

[rtrieu]

Add another exemption for RAII objects in the LHS of the comma operator.

[rtrieu]

Removed the false positives by disabling this warning for the increment expression of a for-loop and in template instantiations. Further testing showed that shrinking the whitelist caught mainly more harmless uses of the comma operator.

[thakis]

This would've found the bug fixed by https://codereview.chromium.org/678193003 . It'd be nice to have this :-)

[rnk]

lgtm

Sorry if this got stuck waiting on me...

[rtrieu]

Updated to be based on newer revision.
Remove most cases of the white list. The only cases that remain are casts to void and the third argument in a for-loop.

[thakis]

Did this ever land? Looks like it's lgtm'd and ready to go, and it looks like a useful warning.

[rtrieu]

Not sure what happened. Will check it out on Monday.

[rtrieu]

So, this week got consumed by -Wconstant-conversion. We'll see how next
week goes.

[ovyalov]

It seems this CL broke LLDB CMake build bot - http://lab.llvm.org:8011/builders/lldb-x86_64-ubuntu-14.04-cmake/builds/11554
Could you take a look?

[rsmith]

This was already fixed in r261285.

[gpakosz]

Hello,

As of today, -Wcomma is not really documented: https://clang.llvm.org/docs/DiagnosticsReference.html#wcomma

The current whitelisted expressions are increments, decrements, assignments, compound assignments, overloaded versions of these operators, and void returning functions

About void returning functions, the following example emits -Wcomma:

void foo()
{

}

int bar(int i)
{
  return i;
}

int main(int argc, char* argv[])
{
  if (foo(), bar(argc))
    return 0;

  return -1;
}

<source>:13:12: warning: possible misuse of comma operator here [-Wcomma]
  if (foo(), bar(argc))
           ^
<source>:13:7: note: cast expression to void to silence warning
  if (foo(), bar(argc))
      ^~~~~
      (void)( )
1 warning generated.

Should I open a bug?

[aaron.ballman]

In D3976#3721421, @gpakosz wrote:

Hello,

As of today, -Wcomma is not really documented: https://clang.llvm.org/docs/DiagnosticsReference.html#wcomma

The current whitelisted expressions are increments, decrements, assignments, compound assignments, overloaded versions of these operators, and void returning functions

FWIW, none of our diagnostics get documentation beyond what you see there (it's auto-generated from the diagnostic definition file).

About void returning functions, the following example emits -Wcomma:

void foo()
{

}

int bar(int i)
{
  return i;
}

int main(int argc, char* argv[])
{
  if (foo(), bar(argc))
    return 0;

  return -1;
}

<source>:13:12: warning: possible misuse of comma operator here [-Wcomma]
  if (foo(), bar(argc))
           ^
<source>:13:7: note: cast expression to void to silence warning
  if (foo(), bar(argc))
      ^~~~~
      (void)( )
1 warning generated.

Should I open a bug?

I think opening a bug is appropriate, good catch!

[gpakosz]

Should I open a bug?

I think opening a bug is appropriate, good catch!

Thanks for the reply Aaron.
Here's the report: https://github.com/llvm/llvm-project/issues/57151

cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td

 def warn_pointer_abs : Warning<
   "taking the absolute value of %select{pointer|function|array}0 type %1 is suspicious">,
   InGroup<AbsoluteValue>;
 
 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">;
 def ext_expr_not_ice : Extension<
   "expression is not an %select{integer|integral}0 constant expression; "
   "folding it to a constant is a GNU extension">, InGroup<GNUFoldingConstant>;
 def err_typecheck_converted_constant_expression : Error<
   "value of type %0 is not implicitly convertible to %1">;

cfe/trunk/include/clang/Sema/Sema.h

 public:
   ExprResult ActOnBinOp(Scope *S, SourceLocation TokLoc,
                         tok::TokenKind Kind, Expr *LHSExpr, Expr *RHSExpr);
   ExprResult BuildBinOp(Scope *S, SourceLocation OpLoc,
                         BinaryOperatorKind Opc, Expr *LHSExpr, Expr *RHSExpr);
   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,
                                 SourceLocation ColonLoc,
                                 Expr *CondExpr, Expr *LHSExpr, Expr *RHSExpr);
 
   /// ActOnAddrLabel - Parse the GNU address of label extension: "&&foo".
   ExprResult ActOnAddrLabel(SourceLocation OpLoc, SourceLocation LabLoc,

cfe/trunk/lib/Sema/SemaExpr.cpp

QualType Sema::CheckAssignmentOperands(Expr *LHSExpr, ExprResult &RHS,
   // C99 6.5.16.1p2: In simple assignment, the value of the right operand
   // is converted to the type of the assignment expression (above).
   // C++ 5.17p1: the type of the assignment expression is that of its left
   // operand.
   return (getLangOpts().CPlusPlus
           ? 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) {
   LHS = S.CheckPlaceholderExpr(LHS.get());
   RHS = S.CheckPlaceholderExpr(RHS.get());
   if (LHS.isInvalid() || RHS.isInvalid())
     return QualType();
 
if (!S.getLangOpts().CPlusPlus) {
     RHS = S.DefaultFunctionArrayLvalueConversion(RHS.get());
     if (RHS.isInvalid())
       return QualType();
     if (!RHS.get()->getType()->isVoidType())
       S.RequireCompleteType(Loc, RHS.get()->getType(),
                             diag::err_incomplete_type);
   }
 
+  if (!S.getDiagnostics().isIgnored(diag::warn_comma_operator, Loc))
+    S.DiagnoseCommaOperator(LHS.get(), Loc);
+
   return RHS.get()->getType();
 }
 
 /// CheckIncrementDecrementOperand - unlike most "Check" methods, this routine
 /// doesn't need to call UsualUnaryConversions or UsualArithmeticConversions.
 static QualType CheckIncrementDecrementOperand(Sema &S, Expr *Op,
                                                ExprValueKind &VK,
                                                ExprObjectKind &OK,

cfe/trunk/lib/Sema/SemaStmt.cpp

 StmtResult Sema::ActOnAttributedStmt(SourceLocation AttrLoc,
                                      ArrayRef<const Attr*> Attrs,
                                      Stmt *SubStmt) {
   // Fill in the declaration and return it.
   AttributedStmt *LS = AttributedStmt::Create(Context, AttrLoc, Attrs, SubStmt);
   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,
                   Stmt *elseStmt) {
   ExprResult CondResult(CondVal.release());
 
   VarDecl *ConditionVar = nullptr;
   if (CondVar) {
     ConditionVar = cast<VarDecl>(CondVar);
     CondResult = CheckConditionVariable(ConditionVar, IfLoc, true);
     CondResult = ActOnFinishFullExpr(CondResult.get(), IfLoc);
   }
   Expr *ConditionExpr = CondResult.getAs<Expr>();
   if (ConditionExpr) {
+
+    if (!Diags.isIgnored(diag::warn_comma_operator,
+                         ConditionExpr->getExprLoc()))
+      CommaVisitor(*this).Visit(ConditionExpr);
+
     DiagnoseUnusedExprResult(thenStmt);
 
     if (!elseStmt) {
       DiagnoseEmptyStmtBody(ConditionExpr->getLocEnd(), thenStmt,
                             diag::warn_empty_if_body);
     }
 
     DiagnoseUnusedExprResult(elseStmt);
if (CondVar) {
     if (CondResult.isInvalid())
       return StmtError();
   }
   Expr *ConditionExpr = CondResult.get();
   if (!ConditionExpr)
     return StmtError();
   CheckBreakContinueBinding(ConditionExpr);
 
+  if (ConditionExpr &&
+      !Diags.isIgnored(diag::warn_comma_operator, ConditionExpr->getExprLoc()))
+    CommaVisitor(*this).Visit(ConditionExpr);
+
   DiagnoseUnusedExprResult(Body);
 
   if (isa<NullStmt>(Body))
     getCurCompoundScope().setHasEmptyLoopBodies();
 
   return new (Context)
       WhileStmt(Context, ConditionVar, ConditionExpr, Body, WhileLoc);
 }
Sema::ActOnForStmt(SourceLocation ForLoc, SourceLocation LParenLoc,
   if (secondVar) {
     ConditionVar = cast<VarDecl>(secondVar);
     SecondResult = CheckConditionVariable(ConditionVar, ForLoc, true);
     SecondResult = ActOnFinishFullExpr(SecondResult.get(), ForLoc);
     if (SecondResult.isInvalid())
       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);
   DiagnoseUnusedExprResult(Third);
   DiagnoseUnusedExprResult(Body);
 
   if (isa<NullStmt>(Body))
     getCurCompoundScope().setHasEmptyLoopBodies();

cfe/trunk/test/SemaCXX/warn-comma-operator.cpp

+// 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}:")"
+}