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

[YingChi Long via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGccbc22cd8976: [Sema] fix false -Wcomma being emitted from 
void returning functions (authored by inclyc).

Changed prior to commit:
  https://reviews.llvm.org/D131892?vs=452767=452970#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131892/new/

https://reviews.llvm.org/D131892

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/Sema/SemaExpr.cpp
  clang/test/SemaCXX/warn-comma-operator.cpp

Index: clang/test/SemaCXX/warn-comma-operator.cpp
===
--- clang/test/SemaCXX/warn-comma-operator.cpp
+++ clang/test/SemaCXX/warn-comma-operator.cpp
@@ -140,6 +140,27 @@
   // CHECK: fix-it:{{.*}}:{[[@LINE-8]]:46-[[@LINE-8]]:46}:")"
 }
 
+
+void void_func();
+int int_func() { return 0; }
+
+void void_function_comma(){
+  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
+}
+
+typedef void Void;
+Void typedef_func();
+
+void whatever() {
+  // 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();
+}
+
 #ifdef __cplusplus
 class S2 {
 public:
@@ -296,4 +317,23 @@
   (void)T{}, 0;
   static_cast(T{}), 0;
 }
+
+namespace {
+
+// issue #57151
+
+struct S {
+  void mem() {}
+};
+
+void whatever() {
+  struct S s;
+  // Member function calls also work as expected.
+  s.mem(), int_func();
+  // As do lambda calls.
+  []() { return; }(), int_func();
+}
+
+} // namespace
+
 #endif  // ifdef __cplusplus
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -13957,8 +13957,10 @@
   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 ) {
   E = E->IgnoreParens();
 
   if (const CastExpr *CE = dyn_cast(E)) {
@@ -13973,6 +13975,8 @@
 }
   }
 
+  if (const auto *CE = dyn_cast(E))
+return CE->getCallReturnType(Context)->isVoidType();
   return false;
 }
 
@@ -14014,7 +14018,7 @@
   }
 
   // Only allow some expressions on LHS to not warn.
-  if (IgnoreCommaOperand(LHS))
+  if (IgnoreCommaOperand(LHS, Context))
 return;
 
   Diag(Loc, diag::warn_comma_operator);
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -73,7 +73,8 @@
   number of arguments cause an assertion fault.
 - Fix multi-level pack expansion of undeclared function parameters.
   This fixes `Issue 56094 `_.
-
+- Fix `#57151 `_.
+  ``-Wcomma`` is emitted for void returning functions.
 
 Improvements to Clang's diagnostics
 ^^^
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a subscriber: tstellar.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

LGTM!




Comment at: clang/docs/ReleaseNotes.rst:74
   number of arguments cause an assertion fault.
+- Fix `#57151 `_.
+  ``-Wcomma`` is emitted for void returning functions.

inclyc wrote:
> 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 ` `# xxx` in this context.
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).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131892/new/

https://reviews.llvm.org/D131892

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[YingChi Long via Phabricator via cfe-commits]

inclyc added inline comments.



Comment at: clang/docs/ReleaseNotes.rst:74
   number of arguments cause an assertion fault.
+- Fix `#57151 `_.
+  ``-Wcomma`` is emitted for void returning functions.

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 ` `# xxx` in this context.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131892/new/

https://reviews.llvm.org/D131892

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[YingChi Long via Phabricator via cfe-commits]

inclyc updated this revision to Diff 452767.
inclyc added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131892/new/

https://reviews.llvm.org/D131892

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/Sema/SemaExpr.cpp
  clang/test/SemaCXX/warn-comma-operator.cpp

Index: clang/test/SemaCXX/warn-comma-operator.cpp
===
--- clang/test/SemaCXX/warn-comma-operator.cpp
+++ clang/test/SemaCXX/warn-comma-operator.cpp
@@ -140,6 +140,27 @@
   // CHECK: fix-it:{{.*}}:{[[@LINE-8]]:46-[[@LINE-8]]:46}:")"
 }
 
+
+void void_func();
+int int_func() { return 0; }
+
+void void_function_comma(){
+  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
+}
+
+typedef void Void;
+Void typedef_func();
+
+void whatever() {
+  // 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();
+}
+
 #ifdef __cplusplus
 class S2 {
 public:
@@ -296,4 +317,23 @@
   (void)T{}, 0;
   static_cast(T{}), 0;
 }
+
+namespace {
+
+// issue #57151
+
+struct S {
+  void mem() {}
+};
+
+void whatever() {
+  struct S s;
+  // Member function calls also work as expected.
+  s.mem(), int_func();
+  // As do lambda calls.
+  []() { return; }(), int_func();
+}
+
+} // namespace
+
 #endif  // ifdef __cplusplus
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -13957,8 +13957,10 @@
   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 ) {
   E = E->IgnoreParens();
 
   if (const CastExpr *CE = dyn_cast(E)) {
@@ -13973,6 +13975,8 @@
 }
   }
 
+  if (const auto *CE = dyn_cast(E))
+return CE->getCallReturnType(Context)->isVoidType();
   return false;
 }
 
@@ -14014,7 +14018,7 @@
   }
 
   // Only allow some expressions on LHS to not warn.
-  if (IgnoreCommaOperand(LHS))
+  if (IgnoreCommaOperand(LHS, Context))
 return;
 
   Diag(Loc, diag::warn_comma_operator);
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -71,6 +71,8 @@
 - Fix `#57008 `_ - Builtin
   C++ language extension type traits instantiated by a template with unexpected
   number of arguments cause an assertion fault.
+- Fix `#57151 `_.
+  ``-Wcomma`` is emitted for void returning functions.
 
 Improvements to Clang's diagnostics
 ^^^
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

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.




Comment at: clang/lib/Sema/SemaExpr.cpp:13978-13980
+  if (const CallExpr *CE = dyn_cast(E))
+if (const Type *T = CE->getCallReturnType(Context).getTypePtrOrNull())
+  return T->isVoidType();





Comment at: clang/test/SemaCXX/warn-comma-operator.cpp:151
+  // Descriptions about -Wcomma: https://reviews.llvm.org/D3976
+}
+

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();
}
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131892/new/

https://reviews.llvm.org/D131892

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits