[PATCH] D146376: Update static_assert message for redundant cases

[Aaron Ballman via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG06c6fac696e4: Update static_assert message for redundant 
cases (authored by Krishna-13-cyber, committed by aaron.ballman).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146376

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/test/SemaCXX/static-assert.cpp


Index: clang/test/SemaCXX/static-assert.cpp
===
--- clang/test/SemaCXX/static-assert.cpp
+++ clang/test/SemaCXX/static-assert.cpp
@@ -258,8 +258,14 @@
   constexpr bool invert(bool b) {
 return !b;
   }
-  static_assert(invert(true) == invert(false), ""); // expected-error 
{{failed}} \
+
+  static_assert(invert(true) || invert(true), ""); // expected-error {{static 
assertion failed due to requirement 'invert(true) || invert(true)'}}
+  static_assert(invert(true) == invert(false), ""); // expected-error {{static 
assertion failed due to requirement 'invert(true) == invert(false)'}} \
 // expected-note 
{{evaluates to 'false == true'}}
+  static_assert(true && false, ""); // expected-error {{static assertion 
failed due to requirement 'true && false'}}
+  static_assert(invert(true) || invert(true) || false, ""); // expected-error 
{{static assertion failed due to requirement 'invert(true) || invert(true) || 
false'}}
+  static_assert((true && invert(true)) || false, ""); // expected-error 
{{static assertion failed due to requirement '(true && invert(true)) || false'}}
+  static_assert(true && invert(false) && invert(true), ""); // expected-error 
{{static assertion failed due to requirement 'invert(true)'}}
 
   /// No notes here since we compare a bool expression with a bool literal.
   static_assert(invert(true) == true, ""); // expected-error {{failed}}
Index: clang/lib/Sema/SemaDeclCXX.cpp
===
--- clang/lib/Sema/SemaDeclCXX.cpp
+++ clang/lib/Sema/SemaDeclCXX.cpp
@@ -16718,7 +16718,8 @@
 /// Try to print more useful information about a failed static_assert
 /// with expression \E
 void Sema::DiagnoseStaticAssertDetails(const Expr *E) {
-  if (const auto *Op = dyn_cast(E)) {
+  if (const auto *Op = dyn_cast(E);
+  Op && Op->getOpcode() != BO_LOr) {
 const Expr *LHS = Op->getLHS()->IgnoreParenImpCasts();
 const Expr *RHS = Op->getRHS()->IgnoreParenImpCasts();
 
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -213,6 +213,9 @@
 - There were some cases in which the diagnostic for the unavailable attribute
   might not be issued, this fixes those cases.
   (`61815 `_)
+- Clang now avoids unnecessary diagnostic warnings for obvious expressions in
+  the case of binary operators with logical OR operations.
+  (`#57906 `_)
 
 Bug Fixes in This Version
 -


Index: clang/test/SemaCXX/static-assert.cpp
===
--- clang/test/SemaCXX/static-assert.cpp
+++ clang/test/SemaCXX/static-assert.cpp
@@ -258,8 +258,14 @@
   constexpr bool invert(bool b) {
 return !b;
   }
-  static_assert(invert(true) == invert(false), ""); // expected-error {{failed}} \
+
+  static_assert(invert(true) || invert(true), ""); // expected-error {{static assertion failed due to requirement 'invert(true) || invert(true)'}}
+  static_assert(invert(true) == invert(false), ""); // expected-error {{static assertion failed due to requirement 'invert(true) == invert(false)'}} \
 // expected-note {{evaluates to 'false == true'}}
+  static_assert(true && false, ""); // expected-error {{static assertion failed due to requirement 'true && false'}}
+  static_assert(invert(true) || invert(true) || false, ""); // expected-error {{static assertion failed due to requirement 'invert(true) || invert(true) || false'}}
+  static_assert((true && invert(true)) || false, ""); // expected-error {{static assertion failed due to requirement '(true && invert(true)) || false'}}
+  static_assert(true && invert(false) && invert(true), ""); // expected-error {{static assertion failed due to requirement 'invert(true)'}}
 
   /// No notes here since we compare a bool expression with a bool literal.
   static_assert(invert(true) == true, ""); // expected-error {{failed}}
Index: clang/lib/Sema/SemaDeclCXX.cpp
===
--- clang/lib/Sema/SemaDeclCXX.cpp
+++ clang/lib/Sema/SemaDeclCXX.cpp
@@ -16718,7 +16718,8 @@
 /// Try to print more useful information about a failed static_assert
 /// with expression \E
 void 

[PATCH] D146376: Update static_assert message for redundant cases

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D146376#4250814 , @xbolva00 wrote:

> The patch description mixes "or" and "and" in examples, please fix it.
>
> SO probably you meant
>
>   :4:1: error: static assertion failed due to requirement 'is_gitlab'
>   static_assert(is_gitlab or is_weekend);
>   ^ ~

Good catch, I'll fix that when landing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146376

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

[PATCH] D146376: Update static_assert message for redundant cases

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.

So.. new diagnostic is:

  :4:1: error: static assertion failed due to requirement 'is_gitlab'
  static_assert(is_gitlab and is_weekend);
  ^ ~

Okay, a dev changes is_gitlab to true, compiles it again and boom, new error: 
static assertion failed due to requirement 'is_weekend'. With previous version 
of this diagnostic one can see all problems immediatelly. So I am not sure if I 
consider this change as a real improvement.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146376

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

[PATCH] D146376: Update static_assert message for redundant cases

[Krishna Narayanan via Phabricator via cfe-commits]

Krishna-13-cyber updated this revision to Diff 511630.
Krishna-13-cyber added a comment.

Thanks a lot everyone for helping me out with my first patch. I have uploaded 
the patch again after rebase.
Yes, It would be great if you could land it on my behalf.
Name: Krishna Narayanan
Email: 

Thanks again!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146376

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/test/SemaCXX/static-assert.cpp


Index: clang/test/SemaCXX/static-assert.cpp
===
--- clang/test/SemaCXX/static-assert.cpp
+++ clang/test/SemaCXX/static-assert.cpp
@@ -258,8 +258,14 @@
   constexpr bool invert(bool b) {
 return !b;
   }
-  static_assert(invert(true) == invert(false), ""); // expected-error 
{{failed}} \
+
+  static_assert(invert(true) || invert(true), ""); // expected-error {{static 
assertion failed due to requirement 'invert(true) || invert(true)'}}
+  static_assert(invert(true) == invert(false), ""); // expected-error {{static 
assertion failed due to requirement 'invert(true) == invert(false)'}} \
 // expected-note 
{{evaluates to 'false == true'}}
+  static_assert(true && false, ""); // expected-error {{static assertion 
failed due to requirement 'true && false'}}
+  static_assert(invert(true) || invert(true) || false, ""); // expected-error 
{{static assertion failed due to requirement 'invert(true) || invert(true) || 
false'}}
+  static_assert((true && invert(true)) || false, ""); // expected-error 
{{static assertion failed due to requirement '(true && invert(true)) || false'}}
+  static_assert(true && invert(false) && invert(true), ""); // expected-error 
{{static assertion failed due to requirement 'invert(true)'}}
 
   /// No notes here since we compare a bool expression with a bool literal.
   static_assert(invert(true) == true, ""); // expected-error {{failed}}
Index: clang/lib/Sema/SemaDeclCXX.cpp
===
--- clang/lib/Sema/SemaDeclCXX.cpp
+++ clang/lib/Sema/SemaDeclCXX.cpp
@@ -16718,7 +16718,8 @@
 /// Try to print more useful information about a failed static_assert
 /// with expression \E
 void Sema::DiagnoseStaticAssertDetails(const Expr *E) {
-  if (const auto *Op = dyn_cast(E)) {
+  if (const auto *Op = dyn_cast(E);
+  Op && Op->getOpcode() != BO_LOr) {
 const Expr *LHS = Op->getLHS()->IgnoreParenImpCasts();
 const Expr *RHS = Op->getRHS()->IgnoreParenImpCasts();
 
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -213,6 +213,9 @@
 - There were some cases in which the diagnostic for the unavailable attribute
   might not be issued, this fixes those cases.
   (`61815 `_)
+- Clang now avoids unnecessary diagnostic warnings for obvious expressions in
+  the case of binary operators with logical OR operations.
+  (`#57906 `_)
 
 Bug Fixes in This Version
 -


Index: clang/test/SemaCXX/static-assert.cpp
===
--- clang/test/SemaCXX/static-assert.cpp
+++ clang/test/SemaCXX/static-assert.cpp
@@ -258,8 +258,14 @@
   constexpr bool invert(bool b) {
 return !b;
   }
-  static_assert(invert(true) == invert(false), ""); // expected-error {{failed}} \
+
+  static_assert(invert(true) || invert(true), ""); // expected-error {{static assertion failed due to requirement 'invert(true) || invert(true)'}}
+  static_assert(invert(true) == invert(false), ""); // expected-error {{static assertion failed due to requirement 'invert(true) == invert(false)'}} \
 // expected-note {{evaluates to 'false == true'}}
+  static_assert(true && false, ""); // expected-error {{static assertion failed due to requirement 'true && false'}}
+  static_assert(invert(true) || invert(true) || false, ""); // expected-error {{static assertion failed due to requirement 'invert(true) || invert(true) || false'}}
+  static_assert((true && invert(true)) || false, ""); // expected-error {{static assertion failed due to requirement '(true && invert(true)) || false'}}
+  static_assert(true && invert(false) && invert(true), ""); // expected-error {{static assertion failed due to requirement 'invert(true)'}}
 
   /// No notes here since we compare a bool expression with a bool literal.
   static_assert(invert(true) == true, ""); // expected-error {{failed}}
Index: clang/lib/Sema/SemaDeclCXX.cpp
===
--- clang/lib/Sema/SemaDeclCXX.cpp
+++ clang/lib/Sema/SemaDeclCXX.cpp
@@ -16718,7 +16718,8 @@
 /// Try to 

[PATCH] D146376: Update static_assert message for redundant cases

[Aaron Ballman via Phabricator via cfe-commits]

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

LGTM aside from a small nit with the release notes. Do you need someone to land 
this on your behalf? If so, please rebase the patch so it applies cleanly to 
trunk and upload it again, and let me know what name and email address you 
would like used for patch attribution.




Comment at: clang/docs/ReleaseNotes.rst:183
+- Clang now avoids unnecessary diagnostic warnings for obvious expressions in
+  the case of binary operators with logical OR operations.
+  (`#57906 `_)




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146376

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

[PATCH] D146376: Update static_assert message for redundant cases

[Krishna Narayanan via Phabricator via cfe-commits]

Krishna-13-cyber updated this revision to Diff 511181.
Krishna-13-cyber added a comment.

- Updated with the diagnostic messages (comments for test cases)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146376

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/test/SemaCXX/static-assert.cpp


Index: clang/test/SemaCXX/static-assert.cpp
===
--- clang/test/SemaCXX/static-assert.cpp
+++ clang/test/SemaCXX/static-assert.cpp
@@ -258,8 +258,14 @@
   constexpr bool invert(bool b) {
 return !b;
   }
-  static_assert(invert(true) == invert(false), ""); // expected-error 
{{failed}} \
+
+  static_assert(invert(true) || invert(true), ""); // expected-error {{static 
assertion failed due to requirement 'invert(true) || invert(true)'}}
+  static_assert(invert(true) == invert(false), ""); // expected-error {{static 
assertion failed due to requirement 'invert(true) == invert(false)'}} \
 // expected-note 
{{evaluates to 'false == true'}}
+  static_assert(true && false, ""); // expected-error {{static assertion 
failed due to requirement 'true && false'}}
+  static_assert(invert(true) || invert(true) || false, ""); // expected-error 
{{static assertion failed due to requirement 'invert(true) || invert(true) || 
false'}}
+  static_assert((true && invert(true)) || false, ""); // expected-error 
{{static assertion failed due to requirement '(true && invert(true)) || false'}}
+  static_assert(true && invert(false) && invert(true), ""); // expected-error 
{{static assertion failed due to requirement 'invert(true)'}}
 
   /// No notes here since we compare a bool expression with a bool literal.
   static_assert(invert(true) == true, ""); // expected-error {{failed}}
Index: clang/lib/Sema/SemaDeclCXX.cpp
===
--- clang/lib/Sema/SemaDeclCXX.cpp
+++ clang/lib/Sema/SemaDeclCXX.cpp
@@ -16715,7 +16715,8 @@
 /// Try to print more useful information about a failed static_assert
 /// with expression \E
 void Sema::DiagnoseStaticAssertDetails(const Expr *E) {
-  if (const auto *Op = dyn_cast(E)) {
+  if (const auto *Op = dyn_cast(E);
+  Op && Op->getOpcode() != BO_LOr) {
 const Expr *LHS = Op->getLHS()->IgnoreParenImpCasts();
 const Expr *RHS = Op->getRHS()->IgnoreParenImpCasts();
 
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -179,6 +179,9 @@
 - Clang now avoids duplicate warnings on unreachable ``[[fallthrough]];`` 
statements
   previously issued from ``-Wunreachable-code`` and 
``-Wunreachable-code-fallthrough``
   by prioritizing ``-Wunreachable-code-fallthrough``.
+- Clang now avoids unnecessary diagnostic warnings for obvious expressions in
+  the case of binary operators with logical OR operations.
+  (`#57906 `_)
 
 Bug Fixes in This Version
 -


Index: clang/test/SemaCXX/static-assert.cpp
===
--- clang/test/SemaCXX/static-assert.cpp
+++ clang/test/SemaCXX/static-assert.cpp
@@ -258,8 +258,14 @@
   constexpr bool invert(bool b) {
 return !b;
   }
-  static_assert(invert(true) == invert(false), ""); // expected-error {{failed}} \
+
+  static_assert(invert(true) || invert(true), ""); // expected-error {{static assertion failed due to requirement 'invert(true) || invert(true)'}}
+  static_assert(invert(true) == invert(false), ""); // expected-error {{static assertion failed due to requirement 'invert(true) == invert(false)'}} \
 // expected-note {{evaluates to 'false == true'}}
+  static_assert(true && false, ""); // expected-error {{static assertion failed due to requirement 'true && false'}}
+  static_assert(invert(true) || invert(true) || false, ""); // expected-error {{static assertion failed due to requirement 'invert(true) || invert(true) || false'}}
+  static_assert((true && invert(true)) || false, ""); // expected-error {{static assertion failed due to requirement '(true && invert(true)) || false'}}
+  static_assert(true && invert(false) && invert(true), ""); // expected-error {{static assertion failed due to requirement 'invert(true)'}}
 
   /// No notes here since we compare a bool expression with a bool literal.
   static_assert(invert(true) == true, ""); // expected-error {{failed}}
Index: clang/lib/Sema/SemaDeclCXX.cpp
===
--- clang/lib/Sema/SemaDeclCXX.cpp
+++ clang/lib/Sema/SemaDeclCXX.cpp
@@ -16715,7 +16715,8 @@
 /// Try to print more useful information about a failed static_assert
 /// with expression \E
 void 

[PATCH] D146376: Update static_assert message for redundant cases

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D146376#4244355 , 
@Krishna-13-cyber wrote:

> - Updated with release note
> - I had tried adding more text to the `expected-error` but it already gives a 
> diagnostic of `static assertion failed due to requirement` currently. If I 
> try additions to `{{failed}}` then we get **error diagnostics expected but 
> not seen**.

Ah, sorry for being unclear! We didn't mean add an additional `expected-error` 
comment, but to modify the ones you have. e.g.,

Currently:

  static_assert(true && false, ""); // expected-error {{failed}}

Changes to:

  static_assert(true && false, ""); // expected-error {{static assertion failed 
due to requirement 'true && false'}}

(or whatever the actual expected full text of the diagnostic is.)

This helps the reviewers to make sure that the diagnostic behavior isn't 
regressing in surprising ways but pass all our tests because we're only looking 
for the word `failed` and considering that passing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146376

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

[PATCH] D146376: Update static_assert message for redundant cases

[Krishna Narayanan via Phabricator via cfe-commits]

Krishna-13-cyber updated this revision to Diff 510909.
Krishna-13-cyber added a comment.

- Updated with release note
- I had tried adding more text to the `expected-error` but it already gives a 
diagnostic of `static assertion failed due to requirement` currently. If I try 
additions to `{{failed}}` then we get **error diagnostics expected but not 
seen**.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146376

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/test/SemaCXX/static-assert.cpp


Index: clang/test/SemaCXX/static-assert.cpp
===
--- clang/test/SemaCXX/static-assert.cpp
+++ clang/test/SemaCXX/static-assert.cpp
@@ -258,8 +258,14 @@
   constexpr bool invert(bool b) {
 return !b;
   }
+
+  static_assert(invert(true) || invert(true), ""); // expected-error {{failed}}
   static_assert(invert(true) == invert(false), ""); // expected-error 
{{failed}} \
 // expected-note 
{{evaluates to 'false == true'}}
+  static_assert(true && false, ""); // expected-error {{failed}}
+  static_assert(invert(true) || invert(true) || false, ""); // expected-error 
{{failed}}
+  static_assert((true && invert(true)) || false, ""); // expected-error 
{{failed}}
+  static_assert(true && invert(false) && invert(true), ""); // expected-error 
{{failed}}
 
   /// No notes here since we compare a bool expression with a bool literal.
   static_assert(invert(true) == true, ""); // expected-error {{failed}}
Index: clang/lib/Sema/SemaDeclCXX.cpp
===
--- clang/lib/Sema/SemaDeclCXX.cpp
+++ clang/lib/Sema/SemaDeclCXX.cpp
@@ -16715,7 +16715,8 @@
 /// Try to print more useful information about a failed static_assert
 /// with expression \E
 void Sema::DiagnoseStaticAssertDetails(const Expr *E) {
-  if (const auto *Op = dyn_cast(E)) {
+  if (const auto *Op = dyn_cast(E);
+  Op && Op->getOpcode() != BO_LOr) {
 const Expr *LHS = Op->getLHS()->IgnoreParenImpCasts();
 const Expr *RHS = Op->getRHS()->IgnoreParenImpCasts();
 
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -179,6 +179,9 @@
 - Clang now avoids duplicate warnings on unreachable ``[[fallthrough]];`` 
statements
   previously issued from ``-Wunreachable-code`` and 
``-Wunreachable-code-fallthrough``
   by prioritizing ``-Wunreachable-code-fallthrough``.
+- Clang now avoids unnecessary diagnostic warnings for obvious expressions in
+  the case of binary operators with logical OR operations.
+  (`#57906 `_)
 
 Bug Fixes in This Version
 -


Index: clang/test/SemaCXX/static-assert.cpp
===
--- clang/test/SemaCXX/static-assert.cpp
+++ clang/test/SemaCXX/static-assert.cpp
@@ -258,8 +258,14 @@
   constexpr bool invert(bool b) {
 return !b;
   }
+
+  static_assert(invert(true) || invert(true), ""); // expected-error {{failed}}
   static_assert(invert(true) == invert(false), ""); // expected-error {{failed}} \
 // expected-note {{evaluates to 'false == true'}}
+  static_assert(true && false, ""); // expected-error {{failed}}
+  static_assert(invert(true) || invert(true) || false, ""); // expected-error {{failed}}
+  static_assert((true && invert(true)) || false, ""); // expected-error {{failed}}
+  static_assert(true && invert(false) && invert(true), ""); // expected-error {{failed}}
 
   /// No notes here since we compare a bool expression with a bool literal.
   static_assert(invert(true) == true, ""); // expected-error {{failed}}
Index: clang/lib/Sema/SemaDeclCXX.cpp
===
--- clang/lib/Sema/SemaDeclCXX.cpp
+++ clang/lib/Sema/SemaDeclCXX.cpp
@@ -16715,7 +16715,8 @@
 /// Try to print more useful information about a failed static_assert
 /// with expression \E
 void Sema::DiagnoseStaticAssertDetails(const Expr *E) {
-  if (const auto *Op = dyn_cast(E)) {
+  if (const auto *Op = dyn_cast(E);
+  Op && Op->getOpcode() != BO_LOr) {
 const Expr *LHS = Op->getLHS()->IgnoreParenImpCasts();
 const Expr *RHS = Op->getRHS()->IgnoreParenImpCasts();
 
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -179,6 +179,9 @@
 - Clang now avoids duplicate warnings on unreachable ``[[fallthrough]];`` statements
   previously issued from ``-Wunreachable-code`` and ``-Wunreachable-code-fallthrough``
   by prioritizing ``-Wunreachable-code-fallthrough``.
+- Clang now avoids unnecessary diagnostic 

[PATCH] D146376: Update static_assert message for redundant cases

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

The changes need a release note and please do add more text to the 
`expected-error` comments so we can see what the diagnostic actually produces 
(instead of just `{{failed}}`).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146376

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

[PATCH] D146376: Update static_assert message for redundant cases

[Krishna Narayanan via Phabricator via cfe-commits]

Krishna-13-cyber updated this revision to Diff 510186.
Krishna-13-cyber added a comment.

- Updated diff with `git clang-format HEAD~1`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146376

Files:
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/test/SemaCXX/static-assert.cpp


Index: clang/test/SemaCXX/static-assert.cpp
===
--- clang/test/SemaCXX/static-assert.cpp
+++ clang/test/SemaCXX/static-assert.cpp
@@ -258,8 +258,14 @@
   constexpr bool invert(bool b) {
 return !b;
   }
+
+  static_assert(invert(true) || invert(true), ""); // expected-error {{failed}}
   static_assert(invert(true) == invert(false), ""); // expected-error 
{{failed}} \
 // expected-note 
{{evaluates to 'false == true'}}
+  static_assert(true && false, ""); // expected-error {{failed}}
+  static_assert(invert(true) || invert(true) || false, ""); // expected-error 
{{failed}}
+  static_assert((true && invert(true)) || false, ""); // expected-error 
{{failed}}
+  static_assert(true && invert(false) && invert(true), ""); // expected-error 
{{failed}}
 
   /// No notes here since we compare a bool expression with a bool literal.
   static_assert(invert(true) == true, ""); // expected-error {{failed}}
Index: clang/lib/Sema/SemaDeclCXX.cpp
===
--- clang/lib/Sema/SemaDeclCXX.cpp
+++ clang/lib/Sema/SemaDeclCXX.cpp
@@ -16715,7 +16715,8 @@
 /// Try to print more useful information about a failed static_assert
 /// with expression \E
 void Sema::DiagnoseStaticAssertDetails(const Expr *E) {
-  if (const auto *Op = dyn_cast(E)) {
+  if (const auto *Op = dyn_cast(E);
+  Op && Op->getOpcode() != BO_LOr) {
 const Expr *LHS = Op->getLHS()->IgnoreParenImpCasts();
 const Expr *RHS = Op->getRHS()->IgnoreParenImpCasts();
 


Index: clang/test/SemaCXX/static-assert.cpp
===
--- clang/test/SemaCXX/static-assert.cpp
+++ clang/test/SemaCXX/static-assert.cpp
@@ -258,8 +258,14 @@
   constexpr bool invert(bool b) {
 return !b;
   }
+
+  static_assert(invert(true) || invert(true), ""); // expected-error {{failed}}
   static_assert(invert(true) == invert(false), ""); // expected-error {{failed}} \
 // expected-note {{evaluates to 'false == true'}}
+  static_assert(true && false, ""); // expected-error {{failed}}
+  static_assert(invert(true) || invert(true) || false, ""); // expected-error {{failed}}
+  static_assert((true && invert(true)) || false, ""); // expected-error {{failed}}
+  static_assert(true && invert(false) && invert(true), ""); // expected-error {{failed}}
 
   /// No notes here since we compare a bool expression with a bool literal.
   static_assert(invert(true) == true, ""); // expected-error {{failed}}
Index: clang/lib/Sema/SemaDeclCXX.cpp
===
--- clang/lib/Sema/SemaDeclCXX.cpp
+++ clang/lib/Sema/SemaDeclCXX.cpp
@@ -16715,7 +16715,8 @@
 /// Try to print more useful information about a failed static_assert
 /// with expression \E
 void Sema::DiagnoseStaticAssertDetails(const Expr *E) {
-  if (const auto *Op = dyn_cast(E)) {
+  if (const auto *Op = dyn_cast(E);
+  Op && Op->getOpcode() != BO_LOr) {
 const Expr *LHS = Op->getLHS()->IgnoreParenImpCasts();
 const Expr *RHS = Op->getRHS()->IgnoreParenImpCasts();
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D146376: Update static_assert message for redundant cases

[Timm Bäder via Phabricator via cfe-commits]

tbaeder added a comment.

Does `git clang-format HEAD~1` not work on your system? It should only format 
the changed parts, not everything.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146376

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

[PATCH] D146376: Update static_assert message for redundant cases

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/test/SemaCXX/static-assert.cpp:266-268
+  static_assert(invert(true) || invert(true) || false, ""); // expected-error 
{{failed}}
+  static_assert((true && invert(true)) || false, ""); // expected-error 
{{failed}}
+  static_assert(true && invert(false) && invert(true), ""); // expected-error 
{{failed}}

We really need to see more of the diagnostic to know whether the behavior is 
reasonable or not.

I was thinking we'd have given a note as to why the failure occurred, but I see 
now that the current behavior doesn't give a note either. Not that I think this 
is related to your patch, but the current behavior seems a bit surprising: 
https://godbolt.org/z/erPGdsanK  Note how the last diagnostic points to exactly 
where the failure comes from but the first two use the entire expression as the 
failure.



Comment at: 
llvm/utils/gn/secondary/clang-tools-extra/clangd/unittests/BUILD.gn:26
 "//clang/lib/Serialization",
+"//clang/lib/Testing",
 "//clang/lib/Tooling",

This seems like an unrelated change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146376

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

[PATCH] D146376: Update static_assert message for redundant cases

[Timm Bäder via Phabricator via cfe-commits]

tbaeder added inline comments.



Comment at: clang/lib/Sema/SemaDeclCXX.cpp:16718
 void Sema::DiagnoseStaticAssertDetails(const Expr *E) {
-  if (const auto *Op = dyn_cast(E)) {
+  if (const auto *Op = dyn_cast(E);Op && Op->getOpcode() != 
BO_LOr) {
 const Expr *LHS = Op->getLHS()->IgnoreParenImpCasts();

This line is over 80 colums now, remember to format it: 
https://llvm.org/docs/Contributing.html#format-patches


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146376

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

[PATCH] D146376: Update static_assert message for redundant cases

[Krishna Narayanan via Phabricator via cfe-commits]

Krishna-13-cyber updated this revision to Diff 509632.
Krishna-13-cyber added a comment.
Herald added subscribers: llvm-commits, kadircet, arphaman.
Herald added a project: LLVM.

- Updated patch
- Added testcases for chained operators




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146376

Files:
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/test/SemaCXX/static-assert.cpp
  llvm/utils/gn/secondary/clang-tools-extra/clangd/unittests/BUILD.gn


Index: llvm/utils/gn/secondary/clang-tools-extra/clangd/unittests/BUILD.gn
===
--- llvm/utils/gn/secondary/clang-tools-extra/clangd/unittests/BUILD.gn
+++ llvm/utils/gn/secondary/clang-tools-extra/clangd/unittests/BUILD.gn
@@ -23,6 +23,7 @@
 "//clang/lib/Lex",
 "//clang/lib/Sema",
 "//clang/lib/Serialization",
+"//clang/lib/Testing",
 "//clang/lib/Tooling",
 "//clang/lib/Tooling/Core",
 "//clang/lib/Tooling/Inclusions",
Index: clang/test/SemaCXX/static-assert.cpp
===
--- clang/test/SemaCXX/static-assert.cpp
+++ clang/test/SemaCXX/static-assert.cpp
@@ -258,8 +258,14 @@
   constexpr bool invert(bool b) {
 return !b;
   }
+
+  static_assert(invert(true) || invert(true), ""); // expected-error {{failed}}
   static_assert(invert(true) == invert(false), ""); // expected-error 
{{failed}} \
 // expected-note 
{{evaluates to 'false == true'}}
+  static_assert(true && false, ""); // expected-error {{failed}}
+  static_assert(invert(true) || invert(true) || false, ""); // expected-error 
{{failed}}
+  static_assert((true && invert(true)) || false, ""); // expected-error 
{{failed}}
+  static_assert(true && invert(false) && invert(true), ""); // expected-error 
{{failed}}
 
   /// No notes here since we compare a bool expression with a bool literal.
   static_assert(invert(true) == true, ""); // expected-error {{failed}}
Index: clang/lib/Sema/SemaDeclCXX.cpp
===
--- clang/lib/Sema/SemaDeclCXX.cpp
+++ clang/lib/Sema/SemaDeclCXX.cpp
@@ -16715,7 +16715,7 @@
 /// Try to print more useful information about a failed static_assert
 /// with expression \E
 void Sema::DiagnoseStaticAssertDetails(const Expr *E) {
-  if (const auto *Op = dyn_cast(E)) {
+  if (const auto *Op = dyn_cast(E);Op && Op->getOpcode() != 
BO_LOr) {
 const Expr *LHS = Op->getLHS()->IgnoreParenImpCasts();
 const Expr *RHS = Op->getRHS()->IgnoreParenImpCasts();
 


Index: llvm/utils/gn/secondary/clang-tools-extra/clangd/unittests/BUILD.gn
===
--- llvm/utils/gn/secondary/clang-tools-extra/clangd/unittests/BUILD.gn
+++ llvm/utils/gn/secondary/clang-tools-extra/clangd/unittests/BUILD.gn
@@ -23,6 +23,7 @@
 "//clang/lib/Lex",
 "//clang/lib/Sema",
 "//clang/lib/Serialization",
+"//clang/lib/Testing",
 "//clang/lib/Tooling",
 "//clang/lib/Tooling/Core",
 "//clang/lib/Tooling/Inclusions",
Index: clang/test/SemaCXX/static-assert.cpp
===
--- clang/test/SemaCXX/static-assert.cpp
+++ clang/test/SemaCXX/static-assert.cpp
@@ -258,8 +258,14 @@
   constexpr bool invert(bool b) {
 return !b;
   }
+
+  static_assert(invert(true) || invert(true), ""); // expected-error {{failed}}
   static_assert(invert(true) == invert(false), ""); // expected-error {{failed}} \
 // expected-note {{evaluates to 'false == true'}}
+  static_assert(true && false, ""); // expected-error {{failed}}
+  static_assert(invert(true) || invert(true) || false, ""); // expected-error {{failed}}
+  static_assert((true && invert(true)) || false, ""); // expected-error {{failed}}
+  static_assert(true && invert(false) && invert(true), ""); // expected-error {{failed}}
 
   /// No notes here since we compare a bool expression with a bool literal.
   static_assert(invert(true) == true, ""); // expected-error {{failed}}
Index: clang/lib/Sema/SemaDeclCXX.cpp
===
--- clang/lib/Sema/SemaDeclCXX.cpp
+++ clang/lib/Sema/SemaDeclCXX.cpp
@@ -16715,7 +16715,7 @@
 /// Try to print more useful information about a failed static_assert
 /// with expression \E
 void Sema::DiagnoseStaticAssertDetails(const Expr *E) {
-  if (const auto *Op = dyn_cast(E)) {
+  if (const auto *Op = dyn_cast(E);Op && Op->getOpcode() != BO_LOr) {
 const Expr *LHS = Op->getLHS()->IgnoreParenImpCasts();
 const Expr *RHS = Op->getRHS()->IgnoreParenImpCasts();
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D146376: Update static_assert message for redundant cases

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/test/SemaCXX/static-assert.cpp:262
+
+  static_assert(invert(true) || invert(true), ""); // expected-error {{failed}}
+

+1 -- please spell out more of the diagnostic wording (I see you're following a 
pattern elsewhere in the file, but we should have enough diagnostic wording to 
know what the diagnostic is.)

It's hard for me to tell from this review because the patch was not uploaded 
with context (next time, please use `-U` when making the patch diff so that 
this context exists), but is there test coverage showing what happens when the 
operators are chained together? e.g.,
```
static_assert(inverted(true) || invert(true) || false, "");
static_assert((true && invert(true)) || false, "");
static_assert(true && inverted(false) && inverted(true), "");
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146376

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

[PATCH] D146376: Update static_assert message for redundant cases

[Krishna Narayanan via Phabricator via cfe-commits]

Krishna-13-cyber updated this revision to Diff 509589.
Krishna-13-cyber edited the summary of this revision.
Krishna-13-cyber added a comment.

This patch aims to remove some redundant cases of static assert messages where 
the expansions are particularly unhelpful. In this particular patch, we have 
ignored the printing of diagnostic warnings for binary operators with logical 
OR operations.
This is done to prioritise and prefer to emit longer diagnostic warnings for 
more important concerns elsewhere.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146376

Files:
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/test/SemaCXX/static-assert.cpp


Index: clang/test/SemaCXX/static-assert.cpp
===
--- clang/test/SemaCXX/static-assert.cpp
+++ clang/test/SemaCXX/static-assert.cpp
@@ -258,9 +258,14 @@
   constexpr bool invert(bool b) {
 return !b;
   }
+
+  static_assert(invert(true) || invert(true), ""); // expected-error {{failed}}
+
   static_assert(invert(true) == invert(false), ""); // expected-error 
{{failed}} \
 // expected-note 
{{evaluates to 'false == true'}}
 
+  static_assert(true && false, ""); // expected-error {{failed}}
+
   /// No notes here since we compare a bool expression with a bool literal.
   static_assert(invert(true) == true, ""); // expected-error {{failed}}
 
Index: clang/lib/Sema/SemaDeclCXX.cpp
===
--- clang/lib/Sema/SemaDeclCXX.cpp
+++ clang/lib/Sema/SemaDeclCXX.cpp
@@ -16715,7 +16715,7 @@
 /// Try to print more useful information about a failed static_assert
 /// with expression \E
 void Sema::DiagnoseStaticAssertDetails(const Expr *E) {
-  if (const auto *Op = dyn_cast(E)) {
+  if (const auto *Op = dyn_cast(E);Op && Op->getOpcode() != 
BO_LOr) {
 const Expr *LHS = Op->getLHS()->IgnoreParenImpCasts();
 const Expr *RHS = Op->getRHS()->IgnoreParenImpCasts();
 


Index: clang/test/SemaCXX/static-assert.cpp
===
--- clang/test/SemaCXX/static-assert.cpp
+++ clang/test/SemaCXX/static-assert.cpp
@@ -258,9 +258,14 @@
   constexpr bool invert(bool b) {
 return !b;
   }
+
+  static_assert(invert(true) || invert(true), ""); // expected-error {{failed}}
+
   static_assert(invert(true) == invert(false), ""); // expected-error {{failed}} \
 // expected-note {{evaluates to 'false == true'}}
 
+  static_assert(true && false, ""); // expected-error {{failed}}
+
   /// No notes here since we compare a bool expression with a bool literal.
   static_assert(invert(true) == true, ""); // expected-error {{failed}}
 
Index: clang/lib/Sema/SemaDeclCXX.cpp
===
--- clang/lib/Sema/SemaDeclCXX.cpp
+++ clang/lib/Sema/SemaDeclCXX.cpp
@@ -16715,7 +16715,7 @@
 /// Try to print more useful information about a failed static_assert
 /// with expression \E
 void Sema::DiagnoseStaticAssertDetails(const Expr *E) {
-  if (const auto *Op = dyn_cast(E)) {
+  if (const auto *Op = dyn_cast(E);Op && Op->getOpcode() != BO_LOr) {
 const Expr *LHS = Op->getLHS()->IgnoreParenImpCasts();
 const Expr *RHS = Op->getRHS()->IgnoreParenImpCasts();
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D146376: Update static_assert message for redundant cases

[Timm Bäder via Phabricator via cfe-commits]

tbaeder added inline comments.



Comment at: clang/lib/Sema/SemaDeclCXX.cpp:16732
+// Ignore BO_LOr operators at the toplevel.
+if (Op->getOpcode() == BO_LOr)
+  return;

tbaeder wrote:
> You can just drop the comment and move this into the `if` statement above, 
> e..g  `if (...; BO && BO->getOpcode() != BO_Lor)`.
I was talking about the one that checks that `E` is a `BinaryOperator`. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146376

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

[PATCH] D146376: Update static_assert message for redundant cases

[Timm Bäder via Phabricator via cfe-commits]

tbaeder added a comment.

Make sure to include context in the patch you upload: 
https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146376

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

[PATCH] D146376: Update static_assert message for redundant cases

[Krishna Narayanan via Phabricator via cfe-commits]

Krishna-13-cyber updated this revision to Diff 509249.
Krishna-13-cyber added a comment.

I have removed the redundant comments and have moved the `if' condition above 
with the ongoing conditional statement (to avoid printing obvious expressions).

- Updated static_assert warning message
- Added Test cases




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146376

Files:
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/test/SemaCXX/static-assert.cpp


Index: clang/test/SemaCXX/static-assert.cpp
===
--- clang/test/SemaCXX/static-assert.cpp
+++ clang/test/SemaCXX/static-assert.cpp
@@ -258,9 +258,14 @@
   constexpr bool invert(bool b) {
 return !b;
   }
+
+  static_assert(invert(true) || invert(true), ""); // expected-error {{failed}}
+
   static_assert(invert(true) == invert(false), ""); // expected-error 
{{failed}} \
 // expected-note 
{{evaluates to 'false == true'}}
 
+  static_assert(true && false, ""); // expected-error {{failed}}
+
   /// No notes here since we compare a bool expression with a bool literal.
   static_assert(invert(true) == true, ""); // expected-error {{failed}}
 
Index: clang/lib/Sema/SemaDeclCXX.cpp
===
--- clang/lib/Sema/SemaDeclCXX.cpp
+++ clang/lib/Sema/SemaDeclCXX.cpp
@@ -16725,7 +16725,7 @@
   return;
 
 // Don't print obvious expressions.
-if (!UsefulToPrintExpr(LHS) && !UsefulToPrintExpr(RHS))
+if ((!UsefulToPrintExpr(LHS) && !UsefulToPrintExpr(RHS)) || 
Op->getOpcode() == BO_LOr)
   return;
 
 struct {


Index: clang/test/SemaCXX/static-assert.cpp
===
--- clang/test/SemaCXX/static-assert.cpp
+++ clang/test/SemaCXX/static-assert.cpp
@@ -258,9 +258,14 @@
   constexpr bool invert(bool b) {
 return !b;
   }
+
+  static_assert(invert(true) || invert(true), ""); // expected-error {{failed}}
+
   static_assert(invert(true) == invert(false), ""); // expected-error {{failed}} \
 // expected-note {{evaluates to 'false == true'}}
 
+  static_assert(true && false, ""); // expected-error {{failed}}
+
   /// No notes here since we compare a bool expression with a bool literal.
   static_assert(invert(true) == true, ""); // expected-error {{failed}}
 
Index: clang/lib/Sema/SemaDeclCXX.cpp
===
--- clang/lib/Sema/SemaDeclCXX.cpp
+++ clang/lib/Sema/SemaDeclCXX.cpp
@@ -16725,7 +16725,7 @@
   return;
 
 // Don't print obvious expressions.
-if (!UsefulToPrintExpr(LHS) && !UsefulToPrintExpr(RHS))
+if ((!UsefulToPrintExpr(LHS) && !UsefulToPrintExpr(RHS)) || Op->getOpcode() == BO_LOr)
   return;
 
 struct {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D146376: Update static_assert message for redundant cases

[Timm Bäder via Phabricator via cfe-commits]

tbaeder added a comment.

Remember to upload you patches with context.




Comment at: clang/lib/Sema/SemaDeclCXX.cpp:16732
+// Ignore BO_LOr operators at the toplevel.
+if (Op->getOpcode() == BO_LOr)
+  return;

You can just drop the comment and move this into the `if` statement above, e..g 
 `if (...; BO && BO->getOpcode() != BO_Lor)`.



Comment at: clang/test/SemaCXX/static-assert.cpp:262
+
+  /// Bools are printed with disjunction.
+  static_assert(invert(true) || invert(true), ""); // expected-error 
{{failed}} \

That comment doesn't make a lot of sense?



Comment at: clang/test/SemaCXX/static-assert.cpp:263
+  /// Bools are printed with disjunction.
+  static_assert(invert(true) || invert(true), ""); // expected-error 
{{failed}} \
+

Remove the backslash



Comment at: clang/test/SemaCXX/static-assert.cpp:269
 
+  /// Bools are printed with conjunction.
+  static_assert(true && false, ""); // expected-error {{failed}} \

Neither this one.



Comment at: clang/test/SemaCXX/static-assert.cpp:270
+  /// Bools are printed with conjunction.
+  static_assert(true && false, ""); // expected-error {{failed}} \
+

Remove the backslash


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146376

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

[PATCH] D146376: Update static_assert message for redundant cases

[Krishna Narayanan via Phabricator via cfe-commits]

Krishna-13-cyber updated this revision to Diff 508297.
Krishna-13-cyber added a comment.

Updated with the new test cases and implemented the logic suggested for 
ignoring the BO_LOr operators at the top level. My sincere apologies @tbaeder 
that I could not place this logic at the first instance.

Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146376

Files:
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/test/SemaCXX/static-assert.cpp


Index: clang/test/SemaCXX/static-assert.cpp
===
--- clang/test/SemaCXX/static-assert.cpp
+++ clang/test/SemaCXX/static-assert.cpp
@@ -258,9 +258,17 @@
   constexpr bool invert(bool b) {
 return !b;
   }
+
+  /// Bools are printed with disjunction.
+  static_assert(invert(true) || invert(true), ""); // expected-error 
{{failed}} \
+
+  /// Bools are printed with an expected note.
   static_assert(invert(true) == invert(false), ""); // expected-error 
{{failed}} \
 // expected-note 
{{evaluates to 'false == true'}}
 
+  /// Bools are printed with conjunction.
+  static_assert(true && false, ""); // expected-error {{failed}} \
+
   /// No notes here since we compare a bool expression with a bool literal.
   static_assert(invert(true) == true, ""); // expected-error {{failed}}
 
Index: clang/lib/Sema/SemaDeclCXX.cpp
===
--- clang/lib/Sema/SemaDeclCXX.cpp
+++ clang/lib/Sema/SemaDeclCXX.cpp
@@ -16728,6 +16728,10 @@
 if (!UsefulToPrintExpr(LHS) && !UsefulToPrintExpr(RHS))
   return;
 
+// Ignore BO_LOr operators at the toplevel.
+if (Op->getOpcode() == BO_LOr)
+  return;
+
 struct {
   const clang::Expr *Cond;
   Expr::EvalResult Result;


Index: clang/test/SemaCXX/static-assert.cpp
===
--- clang/test/SemaCXX/static-assert.cpp
+++ clang/test/SemaCXX/static-assert.cpp
@@ -258,9 +258,17 @@
   constexpr bool invert(bool b) {
 return !b;
   }
+
+  /// Bools are printed with disjunction.
+  static_assert(invert(true) || invert(true), ""); // expected-error {{failed}} \
+
+  /// Bools are printed with an expected note.
   static_assert(invert(true) == invert(false), ""); // expected-error {{failed}} \
 // expected-note {{evaluates to 'false == true'}}
 
+  /// Bools are printed with conjunction.
+  static_assert(true && false, ""); // expected-error {{failed}} \
+
   /// No notes here since we compare a bool expression with a bool literal.
   static_assert(invert(true) == true, ""); // expected-error {{failed}}
 
Index: clang/lib/Sema/SemaDeclCXX.cpp
===
--- clang/lib/Sema/SemaDeclCXX.cpp
+++ clang/lib/Sema/SemaDeclCXX.cpp
@@ -16728,6 +16728,10 @@
 if (!UsefulToPrintExpr(LHS) && !UsefulToPrintExpr(RHS))
   return;
 
+// Ignore BO_LOr operators at the toplevel.
+if (Op->getOpcode() == BO_LOr)
+  return;
+
 struct {
   const clang::Expr *Cond;
   Expr::EvalResult Result;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D146376: Update static_assert message for redundant cases

[Timm Bäder via Phabricator via cfe-commits]

tbaeder added inline comments.



Comment at: clang/test/SemaCXX/static-assert.cpp:262
   static_assert(invert(true) == invert(false), ""); // expected-error 
{{failed}} \
-// expected-note 
{{evaluates to 'false == true'}}
 

Krishna-13-cyber wrote:
> tbaeder wrote:
> > This diagnostic should be kept. From looking at the condition, it is not 
> > obvious what the two functions evaluate to.
> The above Binary operator test case says that there are two Boolean 
> expressions,**UsefulToPrintExpr** says we can avoid to print these 
> expressions as they are don't need a note and are understandable.
> 
> So if we go by this we will have to remove the note.By this we are removing 
> note for boolean literals as well as expressions.It will be nice to make it 
> generic rather than specifically target cases of false || false , false && 
> false. As the warning note print out the boolean values which can be avoided 
> giving preference to the other diagnostics in terms of boolean literals and 
> expressions.
I think you are confusing boolean expressions with boolean literals.

What is the problem with fixing this issue by simply ignoring toplevel `BO_LOr` 
binary operators?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146376

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

[PATCH] D146376: Update static_assert message for redundant cases

[Krishna Narayanan via Phabricator via cfe-commits]

Krishna-13-cyber added a comment.

The above Binary operator test case says that there are two Boolean 
expressions,**UsefulToPrintExpr** says we can avoid to print these expressions 
as they are don't need a note and are understandable.

So if we go by this we will have to remove the note.By this we are removing 
note for boolean literals as well as expressions.It will be nice to make it 
generic rather than specifically target cases of false || false , false && 
false. As the warning note print out the boolean values which can be avoided 
giving preference to the other diagnostics in terms of boolean literals and 
expressions.

Yes, I making a **new diff for test cases** of conjunction and disjunction as 
well.




Comment at: clang/test/SemaCXX/static-assert.cpp:261-265
   static_assert(invert(true) == invert(false), ""); // expected-error 
{{failed}} \
-// expected-note 
{{evaluates to 'false == true'}}
 
   /// No notes here since we compare a bool expression with a bool literal.
   static_assert(invert(true) == true, ""); // expected-error {{failed}}

cjdb wrote:
> We should also have a test for conjunctions, disjunctions, and negations.
Yes, I making a new diff for test cases of conjunction and disjunction as well.



Comment at: clang/test/SemaCXX/static-assert.cpp:262
   static_assert(invert(true) == invert(false), ""); // expected-error 
{{failed}} \
-// expected-note 
{{evaluates to 'false == true'}}
 

tbaeder wrote:
> This diagnostic should be kept. From looking at the condition, it is not 
> obvious what the two functions evaluate to.
The above Binary operator test case says that there are two Boolean 
expressions,**UsefulToPrintExpr** says we can avoid to print these expressions 
as they are don't need a note and are understandable.

So if we go by this we will have to remove the note.By this we are removing 
note for boolean literals as well as expressions.It will be nice to make it 
generic rather than specifically target cases of false || false , false && 
false. As the warning note print out the boolean values which can be avoided 
giving preference to the other diagnostics in terms of boolean literals and 
expressions.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146376

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

[PATCH] D146376: Update static_assert message for redundant cases

[Timm Bäder via Phabricator via cfe-commits]

tbaeder added a comment.

I think just checking that the toplevel binary operator is a logical or should 
be enough? Since that only fails if both operands evaluate to false, so we 
don't need to print the "false or false" diagnostic.




Comment at: clang/test/SemaCXX/static-assert.cpp:262
   static_assert(invert(true) == invert(false), ""); // expected-error 
{{failed}} \
-// expected-note 
{{evaluates to 'false == true'}}
 

This diagnostic should be kept. From looking at the condition, it is not 
obvious what the two functions evaluate to.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146376

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

[PATCH] D146376: Update static_assert message for redundant cases

[Christopher Di Bella via Phabricator via cfe-commits]

cjdb added inline comments.



Comment at: clang/test/SemaCXX/static-assert.cpp:261-265
   static_assert(invert(true) == invert(false), ""); // expected-error 
{{failed}} \
-// expected-note 
{{evaluates to 'false == true'}}
 
   /// No notes here since we compare a bool expression with a bool literal.
   static_assert(invert(true) == true, ""); // expected-error {{failed}}

We should also have a test for conjunctions, disjunctions, and negations.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146376

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

[PATCH] D146376: Update static_assert message for redundant cases

[Krishna Narayanan via Phabricator via cfe-commits]

Krishna-13-cyber updated this revision to Diff 507413.
Krishna-13-cyber added a comment.

Have worked on the top level with Boolean literals unlike the previous diffs.I 
have updated the test case as well with this new upcoming change.

Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146376

Files:
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/test/SemaCXX/static-assert.cpp


Index: clang/test/SemaCXX/static-assert.cpp
===
--- clang/test/SemaCXX/static-assert.cpp
+++ clang/test/SemaCXX/static-assert.cpp
@@ -259,7 +259,6 @@
 return !b;
   }
   static_assert(invert(true) == invert(false), ""); // expected-error 
{{failed}} \
-// expected-note 
{{evaluates to 'false == true'}}
 
   /// No notes here since we compare a bool expression with a bool literal.
   static_assert(invert(true) == true, ""); // expected-error {{failed}}
Index: clang/lib/Sema/SemaDeclCXX.cpp
===
--- clang/lib/Sema/SemaDeclCXX.cpp
+++ clang/lib/Sema/SemaDeclCXX.cpp
@@ -16728,6 +16728,10 @@
 if (!UsefulToPrintExpr(LHS) && !UsefulToPrintExpr(RHS))
   return;
 
+// Don't print obvious boolean literals.
+if (LHS->getType()->isBooleanType() && RHS->getType()->isBooleanType())
+  return;
+
 struct {
   const clang::Expr *Cond;
   Expr::EvalResult Result;


Index: clang/test/SemaCXX/static-assert.cpp
===
--- clang/test/SemaCXX/static-assert.cpp
+++ clang/test/SemaCXX/static-assert.cpp
@@ -259,7 +259,6 @@
 return !b;
   }
   static_assert(invert(true) == invert(false), ""); // expected-error {{failed}} \
-// expected-note {{evaluates to 'false == true'}}
 
   /// No notes here since we compare a bool expression with a bool literal.
   static_assert(invert(true) == true, ""); // expected-error {{failed}}
Index: clang/lib/Sema/SemaDeclCXX.cpp
===
--- clang/lib/Sema/SemaDeclCXX.cpp
+++ clang/lib/Sema/SemaDeclCXX.cpp
@@ -16728,6 +16728,10 @@
 if (!UsefulToPrintExpr(LHS) && !UsefulToPrintExpr(RHS))
   return;
 
+// Don't print obvious boolean literals.
+if (LHS->getType()->isBooleanType() && RHS->getType()->isBooleanType())
+  return;
+
 struct {
   const clang::Expr *Cond;
   Expr::EvalResult Result;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D146376: Update static_assert message for redundant cases

[Timm Bäder via Phabricator via cfe-commits]

tbaeder added a comment.

Since we only handle `BinaryOperator`s in the toplevel assertion expression, I 
think it should just be safe to never diagnose for them if it's an `BO_LOr` 
opcode, so you should be able to just check for that in 
`DiagnoseStaticAssertDetails()`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146376

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

[PATCH] D146376: Update static_assert message for redundant cases

[Krishna Narayanan via Phabricator via cfe-commits]

Krishna-13-cyber updated this revision to Diff 507113.
Krishna-13-cyber added a comment.

I have updated the patch without removing the whole of the previous block.I 
have added a condition to find the case where we don't need a diagnostic 
message to be printed or warned.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146376

Files:
  clang/lib/Sema/SemaDeclCXX.cpp


Index: clang/lib/Sema/SemaDeclCXX.cpp
===
--- clang/lib/Sema/SemaDeclCXX.cpp
+++ clang/lib/Sema/SemaDeclCXX.cpp
@@ -16723,7 +16723,7 @@
 if ((isa(LHS) && RHS->getType()->isBooleanType()) ||
 (isa(RHS) && LHS->getType()->isBooleanType()))
   return;
-
+  
 // Don't print obvious expressions.
 if (!UsefulToPrintExpr(LHS) && !UsefulToPrintExpr(RHS))
   return;
@@ -16744,9 +16744,16 @@
   DiagSide[I].Result.Val, Side->getType(), DiagSide[I].ValueString);
 }
 if (DiagSide[0].Print && DiagSide[1].Print) {
+  if (DiagSide[0].ValueString == SmallString<12>("false") && 
DiagSide[1].ValueString == SmallString<12>("false"))
+  {
+return;
+  }
+  else
+  {
   Diag(Op->getExprLoc(), diag::note_expr_evaluates_to)
   << DiagSide[0].ValueString << Op->getOpcodeStr()
   << DiagSide[1].ValueString << Op->getSourceRange();
+  }
 }
   }
 }


Index: clang/lib/Sema/SemaDeclCXX.cpp
===
--- clang/lib/Sema/SemaDeclCXX.cpp
+++ clang/lib/Sema/SemaDeclCXX.cpp
@@ -16723,7 +16723,7 @@
 if ((isa(LHS) && RHS->getType()->isBooleanType()) ||
 (isa(RHS) && LHS->getType()->isBooleanType()))
   return;
-
+  
 // Don't print obvious expressions.
 if (!UsefulToPrintExpr(LHS) && !UsefulToPrintExpr(RHS))
   return;
@@ -16744,9 +16744,16 @@
   DiagSide[I].Result.Val, Side->getType(), DiagSide[I].ValueString);
 }
 if (DiagSide[0].Print && DiagSide[1].Print) {
+  if (DiagSide[0].ValueString == SmallString<12>("false") && DiagSide[1].ValueString == SmallString<12>("false"))
+  {
+return;
+  }
+  else
+  {
   Diag(Op->getExprLoc(), diag::note_expr_evaluates_to)
   << DiagSide[0].ValueString << Op->getOpcodeStr()
   << DiagSide[1].ValueString << Op->getSourceRange();
+  }
 }
   }
 }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D146376: Update static_assert message for redundant cases

[Christopher Di Bella via Phabricator via cfe-commits]

cjdb added a comment.

Thanks for working on this, it's much appreciated!

  :4:1: error: static assertion failed due to requirement 'is_gitlab'
  static_assert(is_gitlab and is_weekend);
  ^ ~

The arrow seems a bit off here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146376

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

[PATCH] D146376: Update static_assert message for redundant cases

[Timm Bäder via Phabricator via cfe-commits]

tbaeder added a comment.

Looks like you're just removing the output altogether, so that won't work.

Try running `ninja check-clang-semacxx` and see all the test failures you 
(should) get. For a proper patch, it would also be good if you add a test case 
for the case you're fixing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146376

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

[PATCH] D146376: Update static_assert message for redundant cases

[Krishna Narayanan via Phabricator via cfe-commits]

Krishna-13-cyber created this revision.
Krishna-13-cyber added reviewers: tbaeder, cjdb.
Herald added a project: All.
Krishna-13-cyber requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

There are some simple messages where an expansion isn't particularly helpful or 
needed. We aim to eliminate expansions that don't add much value for user (this 
will give us slightly more room or prioritise to have longer diagnostics 
elsewhere).

The test case for which it has been tested:

  constexpr auto is_gitlab = false;
  constexpr auto is_weekend = false;
  static_assert(is_gitlab or is_weekend);

**Previous warning/error message**:

  :4:1: error: static assertion failed due to requirement 'is_gitlab || 
is_weekend'
  static_assert(is_gitlab or is_weekend);
  ^ ~~~
  :4:25: note: expression evaluates to 'false || false'
  static_assert(is_gitlab or is_weekend);
~~^

**Currrent warning/error message**:

  :4:1: error: static assertion failed due to requirement 'is_gitlab'
  static_assert(is_gitlab and is_weekend);
  ^ ~


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D146376

Files:
  clang/lib/Sema/SemaDeclCXX.cpp


Index: clang/lib/Sema/SemaDeclCXX.cpp
===
--- clang/lib/Sema/SemaDeclCXX.cpp
+++ clang/lib/Sema/SemaDeclCXX.cpp
@@ -16744,9 +16744,7 @@
   DiagSide[I].Result.Val, Side->getType(), DiagSide[I].ValueString);
 }
 if (DiagSide[0].Print && DiagSide[1].Print) {
-  Diag(Op->getExprLoc(), diag::note_expr_evaluates_to)
-  << DiagSide[0].ValueString << Op->getOpcodeStr()
-  << DiagSide[1].ValueString << Op->getSourceRange();
+  return;
 }
   }
 }


Index: clang/lib/Sema/SemaDeclCXX.cpp
===
--- clang/lib/Sema/SemaDeclCXX.cpp
+++ clang/lib/Sema/SemaDeclCXX.cpp
@@ -16744,9 +16744,7 @@
   DiagSide[I].Result.Val, Side->getType(), DiagSide[I].ValueString);
 }
 if (DiagSide[0].Print && DiagSide[1].Print) {
-  Diag(Op->getExprLoc(), diag::note_expr_evaluates_to)
-  << DiagSide[0].ValueString << Op->getOpcodeStr()
-  << DiagSide[1].ValueString << Op->getSourceRange();
+  return;
 }
   }
 }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits