[PATCH] D131070: [clang][sema] Fix collectConjunctionTerms()

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

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG8b74074731ee: [clang][sema] Fix collectConjunctionTerms() 
(authored by tbaeder).

Changed prior to commit:
  https://reviews.llvm.org/D131070?vs=449930=450224#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131070

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/Sema/SemaTemplate.cpp


Index: clang/lib/Sema/SemaTemplate.cpp
===
--- clang/lib/Sema/SemaTemplate.cpp
+++ clang/lib/Sema/SemaTemplate.cpp
@@ -3585,9 +3585,8 @@
 if (BinOp->getOpcode() == BO_LAnd) {
   collectConjunctionTerms(BinOp->getLHS(), Terms);
   collectConjunctionTerms(BinOp->getRHS(), Terms);
+  return;
 }
-
-return;
   }
 
   Terms.push_back(Clause);
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -60,6 +60,8 @@
 - No longer assert/miscompile when trying to make a vectorized ``_BitInt`` type
   using the ``ext_vector_type`` attribute (the ``vector_size`` attribute was
   already properly diagnosing this case).
+- Fix clang not properly diagnosing the failing subexpression when chained
+  binary operators are used in a ``static_assert`` expression.
 
 Improvements to Clang's diagnostics
 ^^^


Index: clang/lib/Sema/SemaTemplate.cpp
===
--- clang/lib/Sema/SemaTemplate.cpp
+++ clang/lib/Sema/SemaTemplate.cpp
@@ -3585,9 +3585,8 @@
 if (BinOp->getOpcode() == BO_LAnd) {
   collectConjunctionTerms(BinOp->getLHS(), Terms);
   collectConjunctionTerms(BinOp->getRHS(), Terms);
+  return;
 }
-
-return;
   }
 
   Terms.push_back(Clause);
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -60,6 +60,8 @@
 - No longer assert/miscompile when trying to make a vectorized ``_BitInt`` type
   using the ``ext_vector_type`` attribute (the ``vector_size`` attribute was
   already properly diagnosing this case).
+- Fix clang not properly diagnosing the failing subexpression when chained
+  binary operators are used in a ``static_assert`` expression.
 
 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] D131070: [clang][sema] Fix collectConjunctionTerms()

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

tbaeder added a comment.

FWIW I switched it around so _this_ patch is now good as-is while 
https://reviews.llvm.org/D130894 now depends on this one.


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

https://reviews.llvm.org/D131070

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

[PATCH] D131070: [clang][sema] Fix collectConjunctionTerms()

[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.

Oh shoot, I saw that precommit CI made it past patch application failing (last 
time) and I thought the precommit CI bug was fixed and so it was testing the 
whole stack for once. Drat.

LGTM once we finish up with what this builds on.


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

https://reviews.llvm.org/D131070

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

[PATCH] D131070: [clang][sema] Fix collectConjunctionTerms()

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

tbaeder updated this revision to Diff 449930.

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

https://reviews.llvm.org/D131070

Files:
  clang/lib/Sema/SemaTemplate.cpp


Index: clang/lib/Sema/SemaTemplate.cpp
===
--- clang/lib/Sema/SemaTemplate.cpp
+++ clang/lib/Sema/SemaTemplate.cpp
@@ -3585,9 +3585,8 @@
 if (BinOp->getOpcode() == BO_LAnd) {
   collectConjunctionTerms(BinOp->getLHS(), Terms);
   collectConjunctionTerms(BinOp->getRHS(), Terms);
+  return;
 }
-
-return;
   }
 
   Terms.push_back(Clause);


Index: clang/lib/Sema/SemaTemplate.cpp
===
--- clang/lib/Sema/SemaTemplate.cpp
+++ clang/lib/Sema/SemaTemplate.cpp
@@ -3585,9 +3585,8 @@
 if (BinOp->getOpcode() == BO_LAnd) {
   collectConjunctionTerms(BinOp->getLHS(), Terms);
   collectConjunctionTerms(BinOp->getRHS(), Terms);
+  return;
 }
-
-return;
   }
 
   Terms.push_back(Clause);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131070: [clang][sema] Fix collectConjunctionTerms()

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

tbaeder added a comment.

That's just because of the note that  https://reviews.llvm.org/D130894 adds, 
which the patch expects.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131070

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

[PATCH] D131070: [clang][sema] Fix collectConjunctionTerms()

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

It looks like precommit CI caught a relevant issue


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131070

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

[PATCH] D131070: [clang][sema] Fix collectConjunctionTerms()

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

tbaeder created this revision.
tbaeder added a reviewer: aaron.ballman.
Herald added a project: All.
tbaeder requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Consider:

A == 5 && A != 5

IfA is 5, the old collectConjunctionTerms() would call itself again for
the LHS (which it ignores), then  the RHS (which it also ignores) and
then just return without ever adding anything to the Terms array.

For example, there's a test case in `clang/test/SemaCXX/recovery-expr-type.cpp`:

  namespace test13 {
  enum Circular { // expected-note {{not complete until the closing 
'}'}}
Circular_A = Circular(1), // expected-error {{'Circular' is an incomplete 
type}}
  };
  // Enumerators can be evaluated (they evaluate as zero, but we don't care).
  static_assert(Circular_A == 0 && Circular_A != 0, ""); // expected-error 
{{static assertion failed}}
  }

which currently prints:

  test2.cpp:6:1: error: static assertion failed due to requirement 'Circular_A 
== 0 && Circular_A != 0':
  static_assert(Circular_A == 0 && Circular_A != 0, ""); // expected-error 
{{static assertion failed}}
  ^ ~~
  2 errors generated.

(other diagnostics omitted)

but after this change prints:

  test2.cpp:6:1: error: static assertion failed due to requirement 'Circular_A 
!= 0':
  static_assert(Circular_A == 0 && Circular_A != 0, ""); // expected-error 
{{static assertion failed}}
  ^~~~
  2 errors generated.

The patch depends on https://reviews.llvm.org/D130894 because of the note it 
adds, but that's not necessary. It's just easier because they are both in my 
local tree.

I wanted to add Douglas Gregor as reviewer as well but seems like he isn't 
around anymore.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D131070

Files:
  clang/lib/Sema/SemaTemplate.cpp
  clang/test/SemaCXX/recovery-expr-type.cpp


Index: clang/test/SemaCXX/recovery-expr-type.cpp
===
--- clang/test/SemaCXX/recovery-expr-type.cpp
+++ clang/test/SemaCXX/recovery-expr-type.cpp
@@ -149,7 +149,8 @@
   Circular_A = Circular(1), // expected-error {{'Circular' is an incomplete 
type}}
 };
 // Enumerators can be evaluated (they evaluate as zero, but we don't care).
-static_assert(Circular_A == 0 && Circular_A != 0, ""); // expected-error 
{{static assertion failed}}
+static_assert(Circular_A == 0 && Circular_A != 0, ""); // expected-error 
{{static assertion failed}} \
+   // expected-note 
{{evaluates to 0}}
 }
 
 namespace test14 {
Index: clang/lib/Sema/SemaTemplate.cpp
===
--- clang/lib/Sema/SemaTemplate.cpp
+++ clang/lib/Sema/SemaTemplate.cpp
@@ -3585,9 +3585,8 @@
 if (BinOp->getOpcode() == BO_LAnd) {
   collectConjunctionTerms(BinOp->getLHS(), Terms);
   collectConjunctionTerms(BinOp->getRHS(), Terms);
+  return;
 }
-
-return;
   }
 
   Terms.push_back(Clause);


Index: clang/test/SemaCXX/recovery-expr-type.cpp
===
--- clang/test/SemaCXX/recovery-expr-type.cpp
+++ clang/test/SemaCXX/recovery-expr-type.cpp
@@ -149,7 +149,8 @@
   Circular_A = Circular(1), // expected-error {{'Circular' is an incomplete type}}
 };
 // Enumerators can be evaluated (they evaluate as zero, but we don't care).
-static_assert(Circular_A == 0 && Circular_A != 0, ""); // expected-error {{static assertion failed}}
+static_assert(Circular_A == 0 && Circular_A != 0, ""); // expected-error {{static assertion failed}} \
+   // expected-note {{evaluates to 0}}
 }
 
 namespace test14 {
Index: clang/lib/Sema/SemaTemplate.cpp
===
--- clang/lib/Sema/SemaTemplate.cpp
+++ clang/lib/Sema/SemaTemplate.cpp
@@ -3585,9 +3585,8 @@
 if (BinOp->getOpcode() == BO_LAnd) {
   collectConjunctionTerms(BinOp->getLHS(), Terms);
   collectConjunctionTerms(BinOp->getRHS(), Terms);
+  return;
 }
-
-return;
   }
 
   Terms.push_back(Clause);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits