[Sema] -Wcomma should not warn for expressions that return void

[arphaman]

Summary

Right now -Wcomma is too strict IMO, we shouldn't warn about expressions that return void.

[rnk]

I thought the intention of -Wcomma was to warn on practically all non-macro uses of the comma operator. I know it's silly to cast void to void, but I seem to recall that this was an intentional style-ish warning like -Wparentheses, which encourages if ((x = y)) for intentional assignments in ifs.

[gparker42]

I thought void-returning functions were supposed to be allowed based on the description in https://reviews.llvm.org/D3976 , but later in that discussion the definition was changed to instead allow almost nothing.

[rtrieu]

Reid is correct, the whitelisted expressions was greatly reduced during code review so only casts to void would disable the warning. While the last review did not have the description updated to reflect this, the committed code does have an accurate description. What is the reason to exclude void expressions now? For the function case, it is more consistent to warn on all function calls since we can't determine if a function returns void just by looking at the call site.

[arphaman]

Ah, I see, so this is more of a stylistic warning rather than "suspicious" use one.

In D34810#795926, @rtrieu wrote:

What is the reason to exclude void expressions now? For the function case, it is more consistent to warn on all function calls since we can't determine if a function returns void just by looking at the call site.

We've had an issue with -Wcomma adoption internally and have thought that this was a bug.

[arphaman]

Abandoning. The current behaviour makes sense. Thanks for the responses!

lib/Sema/SemaExpr.cpp

 static bool IgnoreCommaOperand(const Expr *E) {
   E = E->IgnoreParens();
 
   if (const CastExpr *CE = dyn_cast<CastExpr>(E)) {
     if (CE->getCastKind() == CK_ToVoid) {
       return true;
     }
   }
+  if (E->getType()->isVoidType())
+    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) {

test/SemaCXX/warn-comma-operator.cpp

void test14() {
   // 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}:")"
 }
+
+void returnsVoid();
+
+void noWarningForVoidReturns(int x) {
+  returnsVoid(), x = 0;
+  returnsVoid(),
+      return_four(), // expected-warning {{possible misuse of comma operator here}} \
+                        expected-note{{cast expression to void}}
+      returnsVoid();
+}