[Sema] fix false -Wcomma being emitted from void returning functions

[inclyc]

Summary

Fixes https://github.com/llvm/llvm-project/issues/57151

[inclyc]

comments

[inclyc]

Sync comments of function IgnoreCommaOperand

[inclyc]

typo

[aaron.ballman]

Thank you for working on this fix! I think it's quite close to finished, but it needs some additional test coverage. Also, please add a release note about the fix so users know what's going on.

clang/lib/Sema/SemaExpr.cpp

13978–13980

clang/test/SemaCXX/warn-comma-operator.cpp

151

Can you also add test cases like:

struct S {
  void mem();
};

typedef void Void;
Void typedef_func();

void whatever() {
  S s;
  
  // Member function calls also work as expected.
  s.mem(), int_func();
  // As do lambda calls.
  []() { return; }(), int_func();
  // And we don't get confused about type aliases.
  typedef_func(), int_func();
  // Even function pointers don't confuse us.
  void (*fp)() = void_func;
  fp(), int_func();
}

[inclyc]

Address comments.

Thanks a lot for your suggestion, I noticed that the regression test tested both
C and C++, so I split the test mentioned in the comment into two parts.

[inclyc]

clang/docs/ReleaseNotes.rst

74 ↗

(On Diff #452767)

Do we need an NFC commit to unify the format of issues mentioned here? (Maybe it can be scheduled for a later release). The different ways of citing issues here can be confusing. Issue xxxx # xxx in this context.

[aaron.ballman]

LGTM!

clang/docs/ReleaseNotes.rst

74 ↗

(On Diff #452767)

If we wanted to do something like that, I think we should probably do that as part of the release process as a checklist item (CC @tstellar) but I don't have strong opinions either. We can certainly do it with NFC fixes as folks notice issues if we're fine still having some inconsistency when we release (which I'm fine with, personally).

clang/lib/Sema/SemaExpr.cpp

QualType Sema::CheckAssignmentOperands(Expr *LHSExpr, ExprResult &RHS,
   // qualified type, the value has the unqualified version of the type of the
   // lvalue; additionally, if the lvalue has atomic type, the value has the
   // non-atomic version of the type of the lvalue.
   // C++ 5.17p1: the type of the assignment expression is that of its left
   // operand.
   return getLangOpts().CPlusPlus ? LHSType : LHSType.getAtomicUnqualifiedType();
 }
 
-// Only ignore explicit casts to void.
-static bool IgnoreCommaOperand(const Expr *E) {
+// Scenarios to ignore if expression E is:
+// 1. an explicit cast expression into void
+// 2. a function call expression that returns void
+static bool IgnoreCommaOperand(const Expr *E, const ASTContext &Context) {
   E = E->IgnoreParens();
 
   if (const CastExpr *CE = dyn_cast<CastExpr>(E)) {
     if (CE->getCastKind() == CK_ToVoid) {
       return true;
     }
 
     // static_cast<void> on a dependent type will not show up as CK_ToVoid.
     if (CE->getCastKind() == CK_Dependent && E->getType()->isVoidType() &&
         CE->getSubExpr()->getType()->isDependentType()) {
       return true;
     }
   }
 
+  if (const CallExpr *CE = dyn_cast<CallExpr>(E))
+    if (const Type *T = CE->getCallReturnType(Context).getTypePtrOrNull())
+      return T->isVoidType();
   return false;
 }
 
 // Look for instances where it is likely the comma operator is confused with
 // another operator.  There is an explicit list 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
void Sema::DiagnoseCommaOperator(const Expr *LHS, SourceLocation Loc) {
   // 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))
+  if (IgnoreCommaOperand(LHS, Context))
     return;
 
   Diag(Loc, diag::warn_comma_operator);
   Diag(LHS->getBeginLoc(), diag::note_cast_to_void)
       << LHS->getSourceRange()
       << FixItHint::CreateInsertion(LHS->getBeginLoc(),
                                     LangOpts.CPlusPlus ? "static_cast<void>("
                                                        : "(void)(")

clang/test/SemaCXX/warn-comma-operator.cpp

void test_nested_fixits(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 void_func();
+int int_func() { return 0; }
+
+void buggy(){
+  void_func(), int_func(); // expected no -Wcomma because of the returning type `void` 
+  // Reported by https://github.com/llvm/llvm-project/issues/57151
+  // Descriptions about -Wcomma: https://reviews.llvm.org/D3976
+}
+
 #ifdef __cplusplus
 class S2 {
 public:
   void advance();
 
   S2 operator++();
   S2 operator++(int);
   S2 operator--();