[PATCH] D144285: [Clang] Implement CWG2518 - static_assert(false)

2023-03-07 Thread Corentin Jabot via Phabricator via cfe-commits 
cor3ntin added a comment.

In D144285#4176850 , @rupprecht wrote:

> In D144285#4163004 , @cor3ntin 
> wrote:
>
>> If however we find this change to disruptive, we should inform WG21.
>
> Thanks for the explanation, I think I understand the issue now. I got totally 
> thrown off by the title -- it's not about literally writing 
> `static_assert(false)`, it's about deferring `static_assert` evaluation to 
> template instantiations. Being able to write `static_assert(false)` (or any 
> falsy constant expression) is just the common use case for this.
>
> So possibly the most trivial example of something that used to break, but now 
> builds:
>
>   template 
>   void Fail() {
>   static_assert(false, "my assertion failed");
>   }
>
> ... but will still fail as soon as you invoke `Fail()` somewhere.
>
> It doesn't look like there's a lot of impact from this, and the breakages are 
> corner cases like this. It might be worth mentioning this one case to WG21 
> but I'm not sure what I would change about the wording.

Will do !




Comment at: clang/www/cxx_dr_status.html:14915
   
 https://wg21.link/cwg2518;>2518
 review

rupprecht wrote:
> Is it possible to make this link point to 
> https://cplusplus.github.io/CWG/issues/2518.html? This link is inaccessible 
> to anyone not on ISO.
It will get updated whenever a new core issue list gets published, which will 
hopefully be soon-ish.
This file is generated automatically so changing the link might actually not be 
trivial


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144285

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

[PATCH] D144285: [Clang] Implement CWG2518 - static_assert(false)

2023-03-07 Thread Jordan Rupprecht via Phabricator via cfe-commits 
rupprecht added a comment.

In D144285#4163004 , @cor3ntin wrote:

> If however we find this change to disruptive, we should inform WG21.

Thanks for the explanation, I think I understand the issue now. I got totally 
thrown off by the title -- it's not about literally writing 
`static_assert(false)`, it's about deferring `static_assert` evaluation to 
template instantiations. Being able to write `static_assert(false)` (or any 
falsy constant expression) is just the common use case for this.

So possibly the most trivial example of something that used to break, but now 
builds:

  template 
  void Fail() {
  static_assert(false, "my assertion failed");
  }

... but will still fail as soon as you invoke `Fail()` somewhere.

It doesn't look like there's a lot of impact from this, and the breakages are 
corner cases like this. It might be worth mentioning this one case to WG21 but 
I'm not sure what I would change about the wording.




Comment at: clang/www/cxx_dr_status.html:14915
   
 https://wg21.link/cwg2518;>2518
 review

Is it possible to make this link point to 
https://cplusplus.github.io/CWG/issues/2518.html? This link is inaccessible to 
anyone not on ISO.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144285

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

[PATCH] D144285: [Clang] Implement CWG2518 - static_assert(false)

2023-03-01 Thread Corentin Jabot via Phabricator via cfe-commits 
cor3ntin added a comment.

@rupprecht

This expands to

  template 
  class MyMock {
static_assert( static_assert( ::testing::tuple_size >::ArgumentTuple>::value == 2, "This method does not take " 
"2" " arguments. Parenthesize all types with unprotected commas."); 
  };

(some stuff omitted)

Not getting the error here is the expected outcome of this change.
The committee considered that these scenarios were far less likely than the 
scenario where people did not want a diagnostic.
Instantiating `MyMock T;` produces the warning.

I think this code can be changed to something like that 
https://godbolt.org/z/6bcrYfdTf - to partially recover the previous diagnostics.
If however we find this change to disruptive, we should inform WG21.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144285

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

[PATCH] D144285: [Clang] Implement CWG2518 - static_assert(false)

2023-03-01 Thread Jordan Rupprecht via Phabricator via cfe-commits 
rupprecht added a comment.

Here's one change this patch causes on "real" code (invalid code, but something 
a user might try to compile): we see is a static_assert in gmock that now fails 
to report a useful error message: https://godbolt.org/z/sPr1PYT9d

Previously we saw `error: static assertion failed due to requirement 
<...snip...>: This method does not take 2 arguments. Parenthesize all types 
with unprotected commas.`. It still fails to compile, but without the nice 
error message.

My guess, although I haven't looked too closely, is that it's due to the 
`static_assert(false)` here: 
https://github.com/google/googletest/blob/2d4f208765af7fa376b878860a7677ecc0bc390a/googlemock/include/gmock/gmock-function-mocker.h#L149


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144285

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

[PATCH] D144285: [Clang] Implement CWG2518 - static_assert(false)

2023-02-28 Thread Corentin Jabot 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 rG00e2098bf49f: [Clang] Implement CWG2518 - 
static_assert(false) (authored by cor3ntin).

Changed prior to commit:
  https://reviews.llvm.org/D144285?vs=500928=501155#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144285

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/test/CXX/drs/dr25xx.cpp
  clang/test/SemaCXX/access-base-class.cpp
  clang/test/SemaCXX/coroutines.cpp
  clang/test/SemaCXX/static-assert.cpp
  clang/test/SemaTemplate/instantiate-var-template.cpp
  clang/test/SemaTemplate/instantiation-dependence.cpp
  clang/www/cxx_dr_status.html

Index: clang/www/cxx_dr_status.html
===
--- clang/www/cxx_dr_status.html
+++ clang/www/cxx_dr_status.html
@@ -14915,7 +14915,7 @@
 https://wg21.link/cwg2518;>2518
 review
 Conformance requirements and #error/#warning
-Not resolved
+Clang 17
   
   
 https://wg21.link/cwg2519;>2519
Index: clang/test/SemaTemplate/instantiation-dependence.cpp
===
--- clang/test/SemaTemplate/instantiation-dependence.cpp
+++ clang/test/SemaTemplate/instantiation-dependence.cpp
@@ -37,8 +37,8 @@
   template using indirect_void_t = typename indirect_void_t_imp::type;
 
   template void foo() {
-static_assert(!__is_void(indirect_void_t)); // "ok", dependent
-static_assert(!__is_void(void_t)); // expected-error {{failed}}
+int check1[__is_void(indirect_void_t) == 0 ? 1 : -1]; // "ok", dependent
+int check2[__is_void(void_t) == 0 ? 1 : -1]; // expected-error {{array with a negative size}}
   }
 }
 
Index: clang/test/SemaTemplate/instantiate-var-template.cpp
===
--- clang/test/SemaTemplate/instantiate-var-template.cpp
+++ clang/test/SemaTemplate/instantiate-var-template.cpp
@@ -31,8 +31,7 @@
   static_assert(b == 1, ""); // expected-note {{in instantiation of}}
 
   template void f() {
-static_assert(a == 0, ""); // expected-error {{static assertion failed due to requirement 'a == 0'}} \
-   // expected-note {{evaluates to '1 == 0'}}
+int check[a == 0 ? 1 : -1]; // expected-error {{array with a negative size}}
   }
 }
 
Index: clang/test/SemaCXX/static-assert.cpp
===
--- clang/test/SemaCXX/static-assert.cpp
+++ clang/test/SemaCXX/static-assert.cpp
@@ -52,9 +52,11 @@
 
 template struct AlwaysFails {
   // Only give one error here.
-  static_assert(false, ""); // expected-error {{static assertion failed}}
+  static_assert(false, ""); // expected-error 2{{static assertion failed}}
 };
-AlwaysFails alwaysFails;
+AlwaysFails alwaysFails; // expected-note {{instantiation}}
+AlwaysFails alwaysFails2; // expected-note {{instantiation}}
+
 
 template struct StaticAssertProtected {
   static_assert(__is_literal(T), ""); // expected-error {{static assertion failed}}
@@ -217,6 +219,23 @@
 
 static_assert(1 , "") // expected-error {{expected ';' after 'static_assert'}}
 
+namespace DependentAlwaysFalse {
+template 
+struct S {
+  static_assert(false); // expected-error{{static assertion failed}} \
+// expected-warning {{C++17 extension}}
+};
+
+template 
+struct T {
+  static_assert(false, "test"); // expected-error{{static assertion failed: test}}
+};
+
+int f() {
+  S s; //expected-note {{in instantiation of template class 'DependentAlwaysFalse::S' requested here}}
+  T t; //expected-note {{in instantiation of template class 'DependentAlwaysFalse::T' requested here}}
+}
+}
 
 namespace Diagnostics {
   /// No notes for literals.
Index: clang/test/SemaCXX/coroutines.cpp
===
--- clang/test/SemaCXX/coroutines.cpp
+++ clang/test/SemaCXX/coroutines.cpp
@@ -1309,7 +1309,7 @@
   }
 };
 
-template struct DepTestType; // expected-note {{requested here}}
+template struct DepTestType; // expected-note 2{{requested here}}
 template CoroMemberTag DepTestType::test_member_template(long, const char *) const &&;
 
 template CoroMemberTag DepTestType::test_static_template(const char *volatile &, unsigned);
Index: clang/test/SemaCXX/access-base-class.cpp
===
--- clang/test/SemaCXX/access-base-class.cpp
+++ clang/test/SemaCXX/access-base-class.cpp
@@ -96,14 +96,14 @@
 };
 
 template 
-struct trait : flag {};
+struct trait : flag {}; // expected-note 2{{here}}
 
-template ::value>
+template ::value> // expected-note {{here}}
 struct a {};
 
 template 
 class b {
-  a x;
+  a x; // expected-note {{here}}
   using U = a;
 };
 
@@ -113,5 +113,5 @@
 };
 
 //

[PATCH] D144285: [Clang] Implement CWG2518 - static_assert(false)

2023-02-28 Thread Aaron Ballman via Phabricator via cfe-commits 
aaron.ballman accepted this revision.
aaron.ballman added a comment.

LGTM!




Comment at: clang/test/SemaTemplate/instantiate-var-template.cpp:36
   }
+
 }

:: gasps :: spurious whitespace change! :-D


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144285

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

[PATCH] D144285: [Clang] Implement CWG2518 - static_assert(false)

2023-02-27 Thread Corentin Jabot via Phabricator via cfe-commits 
cor3ntin updated this revision to Diff 500928.
cor3ntin added a comment.

Remove the #error / #warning tests


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144285

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/test/CXX/drs/dr25xx.cpp
  clang/test/SemaCXX/access-base-class.cpp
  clang/test/SemaCXX/coroutines-exp-namespace.cpp
  clang/test/SemaCXX/coroutines.cpp
  clang/test/SemaCXX/static-assert.cpp
  clang/test/SemaTemplate/instantiate-var-template.cpp
  clang/test/SemaTemplate/instantiation-dependence.cpp
  clang/www/cxx_dr_status.html

Index: clang/www/cxx_dr_status.html
===
--- clang/www/cxx_dr_status.html
+++ clang/www/cxx_dr_status.html
@@ -14915,7 +14915,7 @@
 https://wg21.link/cwg2518;>2518
 review
 Conformance requirements and #error/#warning
-Not resolved
+Clang 17
   
   
 https://wg21.link/cwg2519;>2519
Index: clang/test/SemaTemplate/instantiation-dependence.cpp
===
--- clang/test/SemaTemplate/instantiation-dependence.cpp
+++ clang/test/SemaTemplate/instantiation-dependence.cpp
@@ -37,8 +37,8 @@
   template using indirect_void_t = typename indirect_void_t_imp::type;
 
   template void foo() {
-static_assert(!__is_void(indirect_void_t)); // "ok", dependent
-static_assert(!__is_void(void_t)); // expected-error {{failed}}
+int check1[__is_void(indirect_void_t) == 0 ? 1 : -1]; // "ok", dependent
+int check2[__is_void(void_t) == 0 ? 1 : -1]; // expected-error {{array with a negative size}}
   }
 }
 
Index: clang/test/SemaTemplate/instantiate-var-template.cpp
===
--- clang/test/SemaTemplate/instantiate-var-template.cpp
+++ clang/test/SemaTemplate/instantiate-var-template.cpp
@@ -31,9 +31,9 @@
   static_assert(b == 1, ""); // expected-note {{in instantiation of}}
 
   template void f() {
-static_assert(a == 0, ""); // expected-error {{static assertion failed due to requirement 'a == 0'}} \
-   // expected-note {{evaluates to '1 == 0'}}
+int check[a == 0 ? 1 : -1]; // expected-error {{array with a negative size}}
   }
+
 }
 
 namespace PR24483 {
Index: clang/test/SemaCXX/static-assert.cpp
===
--- clang/test/SemaCXX/static-assert.cpp
+++ clang/test/SemaCXX/static-assert.cpp
@@ -52,9 +52,11 @@
 
 template struct AlwaysFails {
   // Only give one error here.
-  static_assert(false, ""); // expected-error {{static assertion failed}}
+  static_assert(false, ""); // expected-error 2{{static assertion failed}}
 };
-AlwaysFails alwaysFails;
+AlwaysFails alwaysFails; // expected-note {{instantiation}}
+AlwaysFails alwaysFails2; // expected-note {{instantiation}}
+
 
 template struct StaticAssertProtected {
   static_assert(__is_literal(T), ""); // expected-error {{static assertion failed}}
@@ -217,6 +219,23 @@
 
 static_assert(1 , "") // expected-error {{expected ';' after 'static_assert'}}
 
+namespace DependentAlwaysFalse {
+template 
+struct S {
+  static_assert(false); // expected-error{{static assertion failed}} \
+// expected-warning {{C++17 extension}}
+};
+
+template 
+struct T {
+  static_assert(false, "test"); // expected-error{{static assertion failed: test}}
+};
+
+int f() {
+  S s; //expected-note {{in instantiation of template class 'DependentAlwaysFalse::S' requested here}}
+  T t; //expected-note {{in instantiation of template class 'DependentAlwaysFalse::T' requested here}}
+}
+}
 
 namespace Diagnostics {
   /// No notes for literals.
Index: clang/test/SemaCXX/coroutines.cpp
===
--- clang/test/SemaCXX/coroutines.cpp
+++ clang/test/SemaCXX/coroutines.cpp
@@ -1309,7 +1309,7 @@
   }
 };
 
-template struct DepTestType; // expected-note {{requested here}}
+template struct DepTestType; // expected-note 2{{requested here}}
 template CoroMemberTag DepTestType::test_member_template(long, const char *) const &&;
 
 template CoroMemberTag DepTestType::test_static_template(const char *volatile &, unsigned);
Index: clang/test/SemaCXX/coroutines-exp-namespace.cpp
===
--- clang/test/SemaCXX/coroutines-exp-namespace.cpp
+++ clang/test/SemaCXX/coroutines-exp-namespace.cpp
@@ -1288,7 +1288,7 @@
   }
 };
 
-template struct DepTestType; // expected-note {{requested here}}
+template struct DepTestType; // expected-note 2{{requested here}}
 template CoroMemberTag DepTestType::test_member_template(long, const char *) const &&;
 
 template CoroMemberTag DepTestType::test_static_template(const char *volatile &, unsigned);
Index: clang/test/SemaCXX/access-base-class.cpp

[PATCH] D144285: [Clang] Implement CWG2518 - static_assert(false)

2023-02-27 Thread Aaron Ballman via Phabricator via cfe-commits 
aaron.ballman added inline comments.



Comment at: clang/test/CXX/drs/dr25xx.cpp:5-14
+#error one
+// expected-error@-1 {{one}}
+#if 0
+#error skip
+#warning skip // expected-error {{skip}}
+#endif
+#error two

cor3ntin wrote:
> aaron.ballman wrote:
> > What do these tests have to do with this DR?
> This dr is wild 
> https://wiki.edg.com/pub/Wg21issaquah2023/StrawPolls/p2796r0.html
> CWG merged the static_assert PR in the DR asserting that error should produce 
> a diagnostics - note that there will probably be some follow ups
> https://lists.isocpp.org/core/2023/02/13915.php
> 
> Here I'm testing a warning is emitted even if the build was already failed.
Ah, yeah, CWG2700 is sort of included in here.

I wonder if we should back out the `#error`/`#warning` tests and handle that 
explicitly as part of CWG2700 as that's going to supersede the changes from 
CWG2518.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144285

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

[PATCH] D144285: [Clang] Implement CWG2518 - static_assert(false)

2023-02-27 Thread Corentin Jabot via Phabricator via cfe-commits 
cor3ntin added inline comments.



Comment at: clang/test/CXX/drs/dr25xx.cpp:5-14
+#error one
+// expected-error@-1 {{one}}
+#if 0
+#error skip
+#warning skip // expected-error {{skip}}
+#endif
+#error two

aaron.ballman wrote:
> What do these tests have to do with this DR?
This dr is wild 
https://wiki.edg.com/pub/Wg21issaquah2023/StrawPolls/p2796r0.html
CWG merged the static_assert PR in the DR asserting that error should produce a 
diagnostics - note that there will probably be some follow ups
https://lists.isocpp.org/core/2023/02/13915.php

Here I'm testing a warning is emitted even if the build was already failed.



Comment at: clang/test/CXX/drs/dr25xx.cpp:9
+#error skip
+#warning skip // expected-error {{skip}}
+#endif

aaron.ballman wrote:
> Why do we expect an error on this line in a `#if 0` block??
Oups, we don't 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144285

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

[PATCH] D144285: [Clang] Implement CWG2518 - static_assert(false)

2023-02-27 Thread Corentin Jabot via Phabricator via cfe-commits 
cor3ntin updated this revision to Diff 500866.
cor3ntin marked an inline comment as done.
cor3ntin added a comment.

Address Aaron's comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144285

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/test/CXX/drs/dr25xx.cpp
  clang/test/SemaCXX/access-base-class.cpp
  clang/test/SemaCXX/coroutines-exp-namespace.cpp
  clang/test/SemaCXX/coroutines.cpp
  clang/test/SemaCXX/static-assert.cpp
  clang/test/SemaTemplate/instantiate-var-template.cpp
  clang/test/SemaTemplate/instantiation-dependence.cpp
  clang/www/cxx_dr_status.html

Index: clang/www/cxx_dr_status.html
===
--- clang/www/cxx_dr_status.html
+++ clang/www/cxx_dr_status.html
@@ -14915,7 +14915,7 @@
 https://wg21.link/cwg2518;>2518
 review
 Conformance requirements and #error/#warning
-Not resolved
+Clang 17
   
   
 https://wg21.link/cwg2519;>2519
Index: clang/test/SemaTemplate/instantiation-dependence.cpp
===
--- clang/test/SemaTemplate/instantiation-dependence.cpp
+++ clang/test/SemaTemplate/instantiation-dependence.cpp
@@ -37,8 +37,8 @@
   template using indirect_void_t = typename indirect_void_t_imp::type;
 
   template void foo() {
-static_assert(!__is_void(indirect_void_t)); // "ok", dependent
-static_assert(!__is_void(void_t)); // expected-error {{failed}}
+int check1[__is_void(indirect_void_t) == 0 ? 1 : -1]; // "ok", dependent
+int check2[__is_void(void_t) == 0 ? 1 : -1]; // expected-error {{array with a negative size}}
   }
 }
 
Index: clang/test/SemaTemplate/instantiate-var-template.cpp
===
--- clang/test/SemaTemplate/instantiate-var-template.cpp
+++ clang/test/SemaTemplate/instantiate-var-template.cpp
@@ -31,9 +31,9 @@
   static_assert(b == 1, ""); // expected-note {{in instantiation of}}
 
   template void f() {
-static_assert(a == 0, ""); // expected-error {{static assertion failed due to requirement 'a == 0'}} \
-   // expected-note {{evaluates to '1 == 0'}}
+int check[a == 0 ? 1 : -1]; // expected-error {{array with a negative size}}
   }
+
 }
 
 namespace PR24483 {
Index: clang/test/SemaCXX/static-assert.cpp
===
--- clang/test/SemaCXX/static-assert.cpp
+++ clang/test/SemaCXX/static-assert.cpp
@@ -52,9 +52,11 @@
 
 template struct AlwaysFails {
   // Only give one error here.
-  static_assert(false, ""); // expected-error {{static assertion failed}}
+  static_assert(false, ""); // expected-error 2{{static assertion failed}}
 };
-AlwaysFails alwaysFails;
+AlwaysFails alwaysFails; // expected-note {{instantiation}}
+AlwaysFails alwaysFails2; // expected-note {{instantiation}}
+
 
 template struct StaticAssertProtected {
   static_assert(__is_literal(T), ""); // expected-error {{static assertion failed}}
@@ -217,6 +219,23 @@
 
 static_assert(1 , "") // expected-error {{expected ';' after 'static_assert'}}
 
+namespace DependentAlwaysFalse {
+template 
+struct S {
+  static_assert(false); // expected-error{{static assertion failed}} \
+// expected-warning {{C++17 extension}}
+};
+
+template 
+struct T {
+  static_assert(false, "test"); // expected-error{{static assertion failed: test}}
+};
+
+int f() {
+  S s; //expected-note {{in instantiation of template class 'DependentAlwaysFalse::S' requested here}}
+  T t; //expected-note {{in instantiation of template class 'DependentAlwaysFalse::T' requested here}}
+}
+}
 
 namespace Diagnostics {
   /// No notes for literals.
Index: clang/test/SemaCXX/coroutines.cpp
===
--- clang/test/SemaCXX/coroutines.cpp
+++ clang/test/SemaCXX/coroutines.cpp
@@ -1309,7 +1309,7 @@
   }
 };
 
-template struct DepTestType; // expected-note {{requested here}}
+template struct DepTestType; // expected-note 2{{requested here}}
 template CoroMemberTag DepTestType::test_member_template(long, const char *) const &&;
 
 template CoroMemberTag DepTestType::test_static_template(const char *volatile &, unsigned);
Index: clang/test/SemaCXX/coroutines-exp-namespace.cpp
===
--- clang/test/SemaCXX/coroutines-exp-namespace.cpp
+++ clang/test/SemaCXX/coroutines-exp-namespace.cpp
@@ -1288,7 +1288,7 @@
   }
 };
 
-template struct DepTestType; // expected-note {{requested here}}
+template struct DepTestType; // expected-note 2{{requested here}}
 template CoroMemberTag DepTestType::test_member_template(long, const char *) const &&;
 
 template CoroMemberTag DepTestType::test_static_template(const char *volatile &, unsigned);
Index: clang/test/SemaCXX/access-base-class.cpp

[PATCH] D144285: [Clang] Implement CWG2518 - static_assert(false)

2023-02-27 Thread Aaron Ballman via Phabricator via cfe-commits 
aaron.ballman added inline comments.



Comment at: clang/lib/Sema/SemaDeclCXX.cpp:16824-16841
   if (InnerCond && isa(InnerCond)) {
 // Drill down into concept specialization expressions to see why they
 // weren't satisfied.
 Diag(StaticAssertLoc, diag::err_static_assert_failed)
   << !AssertMessage << Msg.str() << AssertExpr->getSourceRange();
 ConstraintSatisfaction Satisfaction;
 if (!CheckConstraintSatisfaction(InnerCond, Satisfaction))

cor3ntin wrote:
> aaron.ballman wrote:
> > cor3ntin wrote:
> > > rsmith wrote:
> > > > I wonder if it's worth adding a custom diagnostic (eg, "this template 
> > > > cannot be instantiated: %0") for the case where we're in template 
> > > > instantiation and the expression is the bool literal `false`.
> > > I'm not sure i see the motivation. Why would we want to special case 
> > > `false`? The expression could also be an always false, never dependent 
> > > expression
> > Richard may have different ideas in mind, but the motivation to me is code 
> > like: 
> > ```
> > template 
> > struct S {
> >   static_assert(false, "you have to use one of the valid specializations, 
> > not the primary template");
> > };
> > 
> > template <>
> > struct S {
> > };
> > 
> > template <>
> > struct S {
> > };
> > 
> > int main() {
> >   S s1;
> >   S s2;
> >   S s3;
> > }
> > ```
> > Rather than telling the user the static_assert failed because false is not 
> > true, having a custom diagnostic might read better for users. GCC doesn't 
> > produce a custom diagnostic -- the behavior isn't terrible, but the "false 
> > evaluates to false" note is effectively just noise, too: 
> > https://godbolt.org/z/456bzWG7c
> OH. That makes sense now,  thanks. I think I agree.
> Interestingly, in gcc immediate calls are really immediate :) 
> https://godbolt.org/z/b3vrzf4sj 
I think you should add this case as a test in dr25xx.cpp to show we get the 
behavior correct with specializations.



Comment at: clang/test/CXX/drs/dr25xx.cpp:5-14
+#error one
+// expected-error@-1 {{one}}
+#if 0
+#error skip
+#warning skip // expected-error {{skip}}
+#endif
+#error two

What do these tests have to do with this DR?



Comment at: clang/test/CXX/drs/dr25xx.cpp:9
+#error skip
+#warning skip // expected-error {{skip}}
+#endif

Why do we expect an error on this line in a `#if 0` block??



Comment at: clang/test/SemaCXX/static-assert.cpp:235-236
+int f() {
+  S s; //expected-note{{in instantiation of template class 
'DependentAlwaysFalse::S' requested here}}
+  T t; //expected-note{{in instantiation of template class 
'DependentAlwaysFalse::T' requested here}}
+}




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144285

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

[PATCH] D144285: [Clang] Implement CWG2518 - static_assert(false)

2023-02-23 Thread Corentin Jabot via Phabricator via cfe-commits 
cor3ntin updated this revision to Diff 499898.
cor3ntin added a comment.

I played with different diagnostics, we already specialize the
literal case, and I didn't find that not saying a static_assert
failed improved things.
Instead, printing, in addition of that diagnostic, an instantiation
stack provides additional useful information.
(The stack is only printed for the literal bool/integer case)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144285

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/test/CXX/drs/dr25xx.cpp
  clang/test/SemaCXX/access-base-class.cpp
  clang/test/SemaCXX/coroutines-exp-namespace.cpp
  clang/test/SemaCXX/coroutines.cpp
  clang/test/SemaCXX/static-assert.cpp
  clang/test/SemaTemplate/instantiate-var-template.cpp
  clang/test/SemaTemplate/instantiation-dependence.cpp
  clang/www/cxx_dr_status.html

Index: clang/www/cxx_dr_status.html
===
--- clang/www/cxx_dr_status.html
+++ clang/www/cxx_dr_status.html
@@ -14915,7 +14915,7 @@
 https://wg21.link/cwg2518;>2518
 review
 Conformance requirements and #error/#warning
-Not resolved
+Clang 17
   
   
 https://wg21.link/cwg2519;>2519
Index: clang/test/SemaTemplate/instantiation-dependence.cpp
===
--- clang/test/SemaTemplate/instantiation-dependence.cpp
+++ clang/test/SemaTemplate/instantiation-dependence.cpp
@@ -37,8 +37,8 @@
   template using indirect_void_t = typename indirect_void_t_imp::type;
 
   template void foo() {
-static_assert(!__is_void(indirect_void_t)); // "ok", dependent
-static_assert(!__is_void(void_t)); // expected-error {{failed}}
+int check1[__is_void(indirect_void_t) == 0 ? 1 : -1]; // "ok", dependent
+int check2[__is_void(void_t) == 0 ? 1 : -1]; // expected-error {{array with a negative size}}
   }
 }
 
Index: clang/test/SemaTemplate/instantiate-var-template.cpp
===
--- clang/test/SemaTemplate/instantiate-var-template.cpp
+++ clang/test/SemaTemplate/instantiate-var-template.cpp
@@ -31,9 +31,9 @@
   static_assert(b == 1, ""); // expected-note {{in instantiation of}}
 
   template void f() {
-static_assert(a == 0, ""); // expected-error {{static assertion failed due to requirement 'a == 0'}} \
-   // expected-note {{evaluates to '1 == 0'}}
+int check[a == 0 ? 1 : -1]; // expected-error {{array with a negative size}}
   }
+
 }
 
 namespace PR24483 {
Index: clang/test/SemaCXX/static-assert.cpp
===
--- clang/test/SemaCXX/static-assert.cpp
+++ clang/test/SemaCXX/static-assert.cpp
@@ -52,9 +52,11 @@
 
 template struct AlwaysFails {
   // Only give one error here.
-  static_assert(false, ""); // expected-error {{static assertion failed}}
+  static_assert(false, ""); // expected-error 2{{static assertion failed}}
 };
-AlwaysFails alwaysFails;
+AlwaysFails alwaysFails; // expected-note {{instantiation}}
+AlwaysFails alwaysFails2; // expected-note {{instantiation}}
+
 
 template struct StaticAssertProtected {
   static_assert(__is_literal(T), ""); // expected-error {{static assertion failed}}
@@ -217,6 +219,23 @@
 
 static_assert(1 , "") // expected-error {{expected ';' after 'static_assert'}}
 
+namespace DependentAlwaysFalse {
+template 
+struct S {
+  static_assert(false); // expected-error{{static assertion failed}} \
+// expected-warning {{C++17 extension}}
+};
+
+template 
+struct T {
+  static_assert(false, "test"); // expected-error{{static assertion failed: test}}
+};
+
+int f() {
+  S s; //expected-note{{in instantiation of template class 'DependentAlwaysFalse::S' requested here}}
+  T t; //expected-note{{in instantiation of template class 'DependentAlwaysFalse::T' requested here}}
+}
+}
 
 namespace Diagnostics {
   /// No notes for literals.
Index: clang/test/SemaCXX/coroutines.cpp
===
--- clang/test/SemaCXX/coroutines.cpp
+++ clang/test/SemaCXX/coroutines.cpp
@@ -1309,7 +1309,7 @@
   }
 };
 
-template struct DepTestType; // expected-note {{requested here}}
+template struct DepTestType; // expected-note 2{{requested here}}
 template CoroMemberTag DepTestType::test_member_template(long, const char *) const &&;
 
 template CoroMemberTag DepTestType::test_static_template(const char *volatile &, unsigned);
Index: clang/test/SemaCXX/coroutines-exp-namespace.cpp
===
--- clang/test/SemaCXX/coroutines-exp-namespace.cpp
+++ clang/test/SemaCXX/coroutines-exp-namespace.cpp
@@ -1288,7 +1288,7 @@
   }
 };
 
-template struct DepTestType; // expected-note {{requested here}}
+template struct DepTestType; //

[PATCH] D144285: [Clang] Implement CWG2518 - static_assert(false)

2023-02-23 Thread Corentin Jabot via Phabricator via cfe-commits 
cor3ntin added inline comments.



Comment at: clang/lib/Sema/SemaDeclCXX.cpp:16824-16841
   if (InnerCond && isa(InnerCond)) {
 // Drill down into concept specialization expressions to see why they
 // weren't satisfied.
 Diag(StaticAssertLoc, diag::err_static_assert_failed)
   << !AssertMessage << Msg.str() << AssertExpr->getSourceRange();
 ConstraintSatisfaction Satisfaction;
 if (!CheckConstraintSatisfaction(InnerCond, Satisfaction))

aaron.ballman wrote:
> cor3ntin wrote:
> > rsmith wrote:
> > > I wonder if it's worth adding a custom diagnostic (eg, "this template 
> > > cannot be instantiated: %0") for the case where we're in template 
> > > instantiation and the expression is the bool literal `false`.
> > I'm not sure i see the motivation. Why would we want to special case 
> > `false`? The expression could also be an always false, never dependent 
> > expression
> Richard may have different ideas in mind, but the motivation to me is code 
> like: 
> ```
> template 
> struct S {
>   static_assert(false, "you have to use one of the valid specializations, not 
> the primary template");
> };
> 
> template <>
> struct S {
> };
> 
> template <>
> struct S {
> };
> 
> int main() {
>   S s1;
>   S s2;
>   S s3;
> }
> ```
> Rather than telling the user the static_assert failed because false is not 
> true, having a custom diagnostic might read better for users. GCC doesn't 
> produce a custom diagnostic -- the behavior isn't terrible, but the "false 
> evaluates to false" note is effectively just noise, too: 
> https://godbolt.org/z/456bzWG7c
OH. That makes sense now,  thanks. I think I agree.
Interestingly, in gcc immediate calls are really immediate :) 
https://godbolt.org/z/b3vrzf4sj 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144285

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

[PATCH] D144285: [Clang] Implement CWG2518 - static_assert(false)

2023-02-23 Thread Aaron Ballman via Phabricator via cfe-commits 
aaron.ballman added a comment.

In D144285#4135807 , @erichkeane 
wrote:

> In D144285#4135775 , @Endill wrote:
>
>> Thank you for the patch.
>> Any plans to backport this to 16.x branch?
>
> I would not really want us to do that, the breaking change here is 
> concerning, and I'd like this to spend time in trunk 'baking' a while.

+1, at this point the only things we should be backporting to 16.x are fixes 
for regressions or fixes for security concerns.




Comment at: clang/lib/Sema/SemaDeclCXX.cpp:16824-16841
   if (InnerCond && isa(InnerCond)) {
 // Drill down into concept specialization expressions to see why they
 // weren't satisfied.
 Diag(StaticAssertLoc, diag::err_static_assert_failed)
   << !AssertMessage << Msg.str() << AssertExpr->getSourceRange();
 ConstraintSatisfaction Satisfaction;
 if (!CheckConstraintSatisfaction(InnerCond, Satisfaction))

cor3ntin wrote:
> rsmith wrote:
> > I wonder if it's worth adding a custom diagnostic (eg, "this template 
> > cannot be instantiated: %0") for the case where we're in template 
> > instantiation and the expression is the bool literal `false`.
> I'm not sure i see the motivation. Why would we want to special case `false`? 
> The expression could also be an always false, never dependent expression
Richard may have different ideas in mind, but the motivation to me is code 
like: 
```
template 
struct S {
  static_assert(false, "you have to use one of the valid specializations, not 
the primary template");
};

template <>
struct S {
};

template <>
struct S {
};

int main() {
  S s1;
  S s2;
  S s3;
}
```
Rather than telling the user the static_assert failed because false is not 
true, having a custom diagnostic might read better for users. GCC doesn't 
produce a custom diagnostic -- the behavior isn't terrible, but the "false 
evaluates to false" note is effectively just noise, too: 
https://godbolt.org/z/456bzWG7c


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144285

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

[PATCH] D144285: [Clang] Implement CWG2518 - static_assert(false)

2023-02-21 Thread Corentin Jabot via Phabricator via cfe-commits 
cor3ntin added inline comments.



Comment at: clang/docs/ReleaseNotes.rst:62
   directly rather than instantiating the definition from the standard library.
+- Implemented `CWG2518 `_ which allows 
``static_assert(false)``
+  not to be ill-formed when its condition is evaluated in a non-instantiated 
templates.

shafik wrote:
> I am assuming this will be updated eventually but that version is not the 
> final one and the one that was approved can be found from: 
> https://github.com/cplusplus/papers/issues/1251
> 
> and it here: https://cplusplus.github.io/CWG/issues/2518.html
> 
> I think a better wording would be `would not be ill-formed when evaluated in 
> the context of a template definition`
I'll tweak the comment a bit, thanks.
The link points to the yet-to-be-published core issue list so it will 
automatically point to the right thing eventually.



Comment at: clang/lib/Sema/SemaDeclCXX.cpp:16824-16841
   if (InnerCond && isa(InnerCond)) {
 // Drill down into concept specialization expressions to see why they
 // weren't satisfied.
 Diag(StaticAssertLoc, diag::err_static_assert_failed)
   << !AssertMessage << Msg.str() << AssertExpr->getSourceRange();
 ConstraintSatisfaction Satisfaction;
 if (!CheckConstraintSatisfaction(InnerCond, Satisfaction))

rsmith wrote:
> I wonder if it's worth adding a custom diagnostic (eg, "this template cannot 
> be instantiated: %0") for the case where we're in template instantiation and 
> the expression is the bool literal `false`.
I'm not sure i see the motivation. Why would we want to special case `false`? 
The expression could also be an always false, never dependent expression


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144285

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

[PATCH] D144285: [Clang] Implement CWG2518 - static_assert(false)

2023-02-18 Thread Corentin Jabot via Phabricator via cfe-commits 
cor3ntin updated this revision to Diff 498573.
cor3ntin marked 2 inline comments as done.
cor3ntin added a comment.

- Reword Release note
- Restore dependent instantiation tests using Richard's suggestion
- Add additional test to check that diagnostics for static_assert are emitted 
once per instantiation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144285

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/test/CXX/drs/dr25xx.cpp
  clang/test/SemaCXX/access-base-class.cpp
  clang/test/SemaCXX/coroutines-exp-namespace.cpp
  clang/test/SemaCXX/coroutines.cpp
  clang/test/SemaCXX/static-assert.cpp
  clang/test/SemaTemplate/instantiate-var-template.cpp
  clang/test/SemaTemplate/instantiation-dependence.cpp
  clang/www/cxx_dr_status.html

Index: clang/www/cxx_dr_status.html
===
--- clang/www/cxx_dr_status.html
+++ clang/www/cxx_dr_status.html
@@ -14915,7 +14915,7 @@
 https://wg21.link/cwg2518;>2518
 review
 Conformance requirements and #error/#warning
-Not resolved
+Clang 17
   
   
 https://wg21.link/cwg2519;>2519
Index: clang/test/SemaTemplate/instantiation-dependence.cpp
===
--- clang/test/SemaTemplate/instantiation-dependence.cpp
+++ clang/test/SemaTemplate/instantiation-dependence.cpp
@@ -37,8 +37,8 @@
   template using indirect_void_t = typename indirect_void_t_imp::type;
 
   template void foo() {
-static_assert(!__is_void(indirect_void_t)); // "ok", dependent
-static_assert(!__is_void(void_t)); // expected-error {{failed}}
+int check1[__is_void(indirect_void_t) == 0 ? 1 : -1]; // "ok", dependent
+int check2[__is_void(void_t) == 0 ? 1 : -1]; // expected-error {{array with a negative size}}
   }
 }
 
Index: clang/test/SemaTemplate/instantiate-var-template.cpp
===
--- clang/test/SemaTemplate/instantiate-var-template.cpp
+++ clang/test/SemaTemplate/instantiate-var-template.cpp
@@ -31,9 +31,9 @@
   static_assert(b == 1, ""); // expected-note {{in instantiation of}}
 
   template void f() {
-static_assert(a == 0, ""); // expected-error {{static assertion failed due to requirement 'a == 0'}} \
-   // expected-note {{evaluates to '1 == 0'}}
+int check[a == 0 ? 1 : -1]; // expected-error {{array with a negative size}}
   }
+
 }
 
 namespace PR24483 {
Index: clang/test/SemaCXX/static-assert.cpp
===
--- clang/test/SemaCXX/static-assert.cpp
+++ clang/test/SemaCXX/static-assert.cpp
@@ -52,9 +52,11 @@
 
 template struct AlwaysFails {
   // Only give one error here.
-  static_assert(false, ""); // expected-error {{static assertion failed}}
+  static_assert(false, ""); // expected-error 2{{static assertion failed}}
 };
-AlwaysFails alwaysFails;
+AlwaysFails alwaysFails; // expected-note {{instantiation}}
+AlwaysFails alwaysFails2; // expected-note {{instantiation}}
+
 
 template struct StaticAssertProtected {
   static_assert(__is_literal(T), ""); // expected-error {{static assertion failed}}
Index: clang/test/SemaCXX/coroutines.cpp
===
--- clang/test/SemaCXX/coroutines.cpp
+++ clang/test/SemaCXX/coroutines.cpp
@@ -1309,7 +1309,7 @@
   }
 };
 
-template struct DepTestType; // expected-note {{requested here}}
+template struct DepTestType; // expected-note 2{{requested here}}
 template CoroMemberTag DepTestType::test_member_template(long, const char *) const &&;
 
 template CoroMemberTag DepTestType::test_static_template(const char *volatile &, unsigned);
Index: clang/test/SemaCXX/coroutines-exp-namespace.cpp
===
--- clang/test/SemaCXX/coroutines-exp-namespace.cpp
+++ clang/test/SemaCXX/coroutines-exp-namespace.cpp
@@ -1288,7 +1288,7 @@
   }
 };
 
-template struct DepTestType; // expected-note {{requested here}}
+template struct DepTestType; // expected-note 2{{requested here}}
 template CoroMemberTag DepTestType::test_member_template(long, const char *) const &&;
 
 template CoroMemberTag DepTestType::test_static_template(const char *volatile &, unsigned);
Index: clang/test/SemaCXX/access-base-class.cpp
===
--- clang/test/SemaCXX/access-base-class.cpp
+++ clang/test/SemaCXX/access-base-class.cpp
@@ -96,14 +96,14 @@
 };
 
 template 
-struct trait : flag {};
+struct trait : flag {}; // expected-note 2{{here}}
 
-template ::value>
+template ::value> // expected-note {{here}}
 struct a {};
 
 template 
 class b {
-  a x;
+  a x; // expected-note {{here}}
   using U = a;
 };
 
@@ -113,5 +113,5 @@
 };
 
 // verify "no member named 'value'" bogus diagnostic is not

[PATCH] D144285: [Clang] Implement CWG2518 - static_assert(false)

2023-02-17 Thread Richard Smith - zygoloid via Phabricator via cfe-commits 
rsmith added inline comments.



Comment at: clang/lib/Sema/SemaDeclCXX.cpp:16824-16841
   if (InnerCond && isa(InnerCond)) {
 // Drill down into concept specialization expressions to see why they
 // weren't satisfied.
 Diag(StaticAssertLoc, diag::err_static_assert_failed)
   << !AssertMessage << Msg.str() << AssertExpr->getSourceRange();
 ConstraintSatisfaction Satisfaction;
 if (!CheckConstraintSatisfaction(InnerCond, Satisfaction))

I wonder if it's worth adding a custom diagnostic (eg, "this template cannot be 
instantiated: %0") for the case where we're in template instantiation and the 
expression is the bool literal `false`.



Comment at: clang/test/SemaCXX/static-assert.cpp:53-57
 template struct AlwaysFails {
   // Only give one error here.
   static_assert(false, ""); // expected-error {{static assertion failed}}
 };
+AlwaysFails alwaysFails; // expected-note {{instantiation}}

I think we should also test the case where `AlwaysFails` is instantiated twice, 
and that we get one "static assertion failed" error per instantiation, rather 
than only one in total.



Comment at: clang/test/SemaTemplate/instantiate-var-template.cpp:34
   template void f() {
-static_assert(a == 0, ""); // expected-error 
{{static assertion failed due to requirement 'a == 0'}} \
-   // expected-note 
{{evaluates to '1 == 0'}}
+static_assert(a == 0, ""); // fixme: can we check 
a var is dependant?
   }

erichkeane wrote:
> cor3ntin wrote:
> > erichkeane wrote:
> > > You should be able to instantiate this template later, and probably what 
> > > we now have to do.  Also, 'dependent' is the spelling in this case, 
> > > 'dependant' is something different :)
> > I'm afraid doing though would defeat the intent of the test - it is after 
> > all named "InstantiationDependent"
> Ah, yeah, you're right, it isn't clear what this is trying to test to me 
> unfortunately.  It might require some history digging.  Same comment as below 
> on the spelling.
I think you can preserve the intent of this test by using the old array trick 
rather than a static assert:

```
int check[a == 0 ? 1 : -1]; // expected-error {{array 
with a negative size}}
```



Comment at: clang/test/SemaTemplate/instantiation-dependence.cpp:41
 static_assert(!__is_void(indirect_void_t)); // "ok", dependent
-static_assert(!__is_void(void_t)); // expected-error {{failed}}
+static_assert(!__is_void(void_t)); // fixme: can we check a type is 
dependant?
   }

erichkeane wrote:
> This one is probably a bigger pain to test, and I don't have a good idea.
You can use the array trick here too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144285

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

[PATCH] D144285: [Clang] Implement CWG2518 - static_assert(false)

2023-02-17 Thread Erich Keane via Phabricator via cfe-commits 
erichkeane added inline comments.



Comment at: clang/lib/Sema/SemaDeclCXX.cpp:16806
+bool InTemplateDefinition =
+getLangOpts().CPlusPlus && CurContext->isDependentContext();
+

shafik wrote:
> erichkeane wrote:
> > cor3ntin wrote:
> > > erichkeane wrote:
> > > > CplusPlus check is now not really beneficial?  I'm not sure how much it 
> > > > matters now though that these are both just bit-loads.
> > > isDependentContext still does a bunch of work, recursively. I think we 
> > > should keep it!
> > Ah, Oh! You're right!  It is the Expr class where this is free/just 
> > checking a bit.  Disregard.
> So `isDependentContext()` strictly means we are in a definition context? I 
> think it makes sense after reading the implementation but not obvious at 
> first. Maybe that would to document someplace?
Shafik: (if you mean Template Definition Context), yes more or less.  IIRC, 
that term was added to the standard after the clang Templates were implemented 
(I remember someone mentioning a big standards 'template' rewrite a few years 
back), so thats perhaps why it wasn't named that.  You can get the same from an 
expression (isInstantiationDependent, isValueDependent, and isTypeDependent, 
the latter two having language equivalence, and the former just a catch-all).

Feel free to write up some documentation on this in the internals manual, as 
long as you properly explain the Template Definition Context/etc means.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144285

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

[PATCH] D144285: [Clang] Implement CWG2518 - static_assert(false)

2023-02-17 Thread Shafik Yaghmour via Phabricator via cfe-commits 
shafik added inline comments.



Comment at: clang/docs/ReleaseNotes.rst:62
   directly rather than instantiating the definition from the standard library.
+- Implemented `CWG2518 `_ which allows 
``static_assert(false)``
+  not to be ill-formed when its condition is evaluated in a non-instantiated 
templates.

I am assuming this will be updated eventually but that version is not the final 
one and the one that was approved can be found from: 
https://github.com/cplusplus/papers/issues/1251

and it here: https://cplusplus.github.io/CWG/issues/2518.html

I think a better wording would be `would not be ill-formed when evaluated in 
the context of a template definition`



Comment at: clang/lib/Sema/SemaDeclCXX.cpp:16806
+bool InTemplateDefinition =
+getLangOpts().CPlusPlus && CurContext->isDependentContext();
+

erichkeane wrote:
> cor3ntin wrote:
> > erichkeane wrote:
> > > CplusPlus check is now not really beneficial?  I'm not sure how much it 
> > > matters now though that these are both just bit-loads.
> > isDependentContext still does a bunch of work, recursively. I think we 
> > should keep it!
> Ah, Oh! You're right!  It is the Expr class where this is free/just checking 
> a bit.  Disregard.
So `isDependentContext()` strictly means we are in a definition context? I 
think it makes sense after reading the implementation but not obvious at first. 
Maybe that would to document someplace?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144285

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

[PATCH] D144285: [Clang] Implement CWG2518 - static_assert(false)

2023-02-17 Thread Erich Keane via Phabricator via cfe-commits 
erichkeane added inline comments.



Comment at: clang/lib/Sema/SemaDeclCXX.cpp:16806
+bool InTemplateDefinition =
+getLangOpts().CPlusPlus && CurContext->isDependentContext();
+

cor3ntin wrote:
> erichkeane wrote:
> > CplusPlus check is now not really beneficial?  I'm not sure how much it 
> > matters now though that these are both just bit-loads.
> isDependentContext still does a bunch of work, recursively. I think we should 
> keep it!
Ah, Oh! You're right!  It is the Expr class where this is free/just checking a 
bit.  Disregard.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144285

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

[PATCH] D144285: [Clang] Implement CWG2518 - static_assert(false)

2023-02-17 Thread Corentin Jabot via Phabricator via cfe-commits 
cor3ntin added inline comments.



Comment at: clang/lib/Sema/SemaDeclCXX.cpp:16806
+bool InTemplateDefinition =
+getLangOpts().CPlusPlus && CurContext->isDependentContext();
+

erichkeane wrote:
> CplusPlus check is now not really beneficial?  I'm not sure how much it 
> matters now though that these are both just bit-loads.
isDependentContext still does a bunch of work, recursively. I think we should 
keep it!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144285

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

[PATCH] D144285: [Clang] Implement CWG2518 - static_assert(false)

2023-02-17 Thread Erich Keane via Phabricator via cfe-commits 
erichkeane accepted this revision.
erichkeane added inline comments.
This revision is now accepted and ready to land.



Comment at: clang/lib/Sema/SemaDeclCXX.cpp:16806
+bool InTemplateDefinition =
+getLangOpts().CPlusPlus && CurContext->isDependentContext();
+

CplusPlus check is now not really beneficial?  I'm not sure how much it matters 
now though that these are both just bit-loads.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144285

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

[PATCH] D144285: [Clang] Implement CWG2518 - static_assert(false)

2023-02-17 Thread Corentin Jabot via Phabricator via cfe-commits 
cor3ntin marked 2 inline comments as done.
cor3ntin added inline comments.



Comment at: clang/lib/Sema/SemaDeclCXX.cpp:16805
+// definition, the declaration has no effect.
+bool InTemplateDefinition = getLangOpts().CPlusPlus && 
getTemplateDepth(getCurScope()) != 0;
+

erichkeane wrote:
> cor3ntin wrote:
> > erichkeane wrote:
> > > Hmm... interesting way to calculate template depth, I wasn't aware of 
> > > that one.  Does this cause problems in 'a template caused another 
> > > template to instantiate' sorta thing? 
> > > 
> > > Also, is the "CPlusPlus" test here necessary?
> > It's what I came up with. Do you think there is a better way?
> > If you have suggestions for additional tests, I'm all ears!
> > 
> > The  CplusPlus tests is just there to avoid unnecessary cycles in C mode. 
> > It probably doesn't do much of a difference.
> For "Zero" I usually would just check if the current decl-context is 
> dependent.  I'm not positive if that is 'correct' without more thought, 
> though I would expect it to be?  I think we make sure that is set up 
> correctly everywhere.  it would at least be a good check to see where we mess 
> that up :)
Yes, i feel stupid now, that was the obvious solution! Thanks


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144285

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

[PATCH] D144285: [Clang] Implement CWG2518 - static_assert(false)

2023-02-17 Thread Corentin Jabot via Phabricator via cfe-commits 
cor3ntin updated this revision to Diff 498455.
cor3ntin added a comment.

- Simplify how we check we are in a dependent context
- Fix typos


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144285

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/test/CXX/drs/dr25xx.cpp
  clang/test/SemaCXX/access-base-class.cpp
  clang/test/SemaCXX/coroutines-exp-namespace.cpp
  clang/test/SemaCXX/coroutines.cpp
  clang/test/SemaCXX/static-assert.cpp
  clang/test/SemaTemplate/instantiate-var-template.cpp
  clang/test/SemaTemplate/instantiation-dependence.cpp
  clang/www/cxx_dr_status.html

Index: clang/www/cxx_dr_status.html
===
--- clang/www/cxx_dr_status.html
+++ clang/www/cxx_dr_status.html
@@ -14915,7 +14915,7 @@
 https://wg21.link/cwg2518;>2518
 review
 Conformance requirements and #error/#warning
-Not resolved
+Clang 17
   
   
 https://wg21.link/cwg2519;>2519
Index: clang/test/SemaTemplate/instantiation-dependence.cpp
===
--- clang/test/SemaTemplate/instantiation-dependence.cpp
+++ clang/test/SemaTemplate/instantiation-dependence.cpp
@@ -38,7 +38,7 @@
 
   template void foo() {
 static_assert(!__is_void(indirect_void_t)); // "ok", dependent
-static_assert(!__is_void(void_t)); // expected-error {{failed}}
+static_assert(!__is_void(void_t)); // fixme: can we check a type is dependent?
   }
 }
 
Index: clang/test/SemaTemplate/instantiate-var-template.cpp
===
--- clang/test/SemaTemplate/instantiate-var-template.cpp
+++ clang/test/SemaTemplate/instantiate-var-template.cpp
@@ -31,9 +31,9 @@
   static_assert(b == 1, ""); // expected-note {{in instantiation of}}
 
   template void f() {
-static_assert(a == 0, ""); // expected-error {{static assertion failed due to requirement 'a == 0'}} \
-   // expected-note {{evaluates to '1 == 0'}}
+static_assert(a == 0, ""); // fixme: can we check a var is dependent?
   }
+
 }
 
 namespace PR24483 {
Index: clang/test/SemaCXX/static-assert.cpp
===
--- clang/test/SemaCXX/static-assert.cpp
+++ clang/test/SemaCXX/static-assert.cpp
@@ -54,7 +54,7 @@
   // Only give one error here.
   static_assert(false, ""); // expected-error {{static assertion failed}}
 };
-AlwaysFails alwaysFails;
+AlwaysFails alwaysFails; // expected-note {{instantiation}}
 
 template struct StaticAssertProtected {
   static_assert(__is_literal(T), ""); // expected-error {{static assertion failed}}
Index: clang/test/SemaCXX/coroutines.cpp
===
--- clang/test/SemaCXX/coroutines.cpp
+++ clang/test/SemaCXX/coroutines.cpp
@@ -1309,7 +1309,7 @@
   }
 };
 
-template struct DepTestType; // expected-note {{requested here}}
+template struct DepTestType; // expected-note 2{{requested here}}
 template CoroMemberTag DepTestType::test_member_template(long, const char *) const &&;
 
 template CoroMemberTag DepTestType::test_static_template(const char *volatile &, unsigned);
Index: clang/test/SemaCXX/coroutines-exp-namespace.cpp
===
--- clang/test/SemaCXX/coroutines-exp-namespace.cpp
+++ clang/test/SemaCXX/coroutines-exp-namespace.cpp
@@ -1288,7 +1288,7 @@
   }
 };
 
-template struct DepTestType; // expected-note {{requested here}}
+template struct DepTestType; // expected-note 2{{requested here}}
 template CoroMemberTag DepTestType::test_member_template(long, const char *) const &&;
 
 template CoroMemberTag DepTestType::test_static_template(const char *volatile &, unsigned);
Index: clang/test/SemaCXX/access-base-class.cpp
===
--- clang/test/SemaCXX/access-base-class.cpp
+++ clang/test/SemaCXX/access-base-class.cpp
@@ -96,14 +96,14 @@
 };
 
 template 
-struct trait : flag {};
+struct trait : flag {}; // expected-note 2{{here}}
 
-template ::value>
+template ::value> // expected-note {{here}}
 struct a {};
 
 template 
 class b {
-  a x;
+  a x; // expected-note {{here}}
   using U = a;
 };
 
@@ -113,5 +113,5 @@
 };
 
 // verify "no member named 'value'" bogus diagnostic is not emitted.
-trait>>::value;
+trait>>::value; // expected-note {{here}}
 } // namespace T8
Index: clang/test/CXX/drs/dr25xx.cpp
===
--- clang/test/CXX/drs/dr25xx.cpp
+++ clang/test/CXX/drs/dr25xx.cpp
@@ -1,5 +1,33 @@
 // RUN: %clang_cc1 -std=c++20 -triple x86_64-unknown-unknown %s -verify
 
+namespace dr2518 { // dr2518: 17 review
+
+#error one
+// expected-error@-1 {{one}}
+#if 0
+#error skip
+#warning skip // expected-error {{skip}}
+#endif
+#error two

[PATCH] D144285: [Clang] Implement CWG2518 - static_assert(false)

2023-02-17 Thread Erich Keane via Phabricator via cfe-commits 
erichkeane added a comment.

In D144285#4135775 , @Endill wrote:

> Thank you for the patch.
> Any plans to backport this to 16.x branch?

I would not really want us to do that, the breaking change here is concerning, 
and I'd like this to spend time in trunk 'baking' a while.




Comment at: clang/lib/Sema/SemaDeclCXX.cpp:16805
+// definition, the declaration has no effect.
+bool InTemplateDefinition = getLangOpts().CPlusPlus && 
getTemplateDepth(getCurScope()) != 0;
+

cor3ntin wrote:
> erichkeane wrote:
> > Hmm... interesting way to calculate template depth, I wasn't aware of that 
> > one.  Does this cause problems in 'a template caused another template to 
> > instantiate' sorta thing? 
> > 
> > Also, is the "CPlusPlus" test here necessary?
> It's what I came up with. Do you think there is a better way?
> If you have suggestions for additional tests, I'm all ears!
> 
> The  CplusPlus tests is just there to avoid unnecessary cycles in C mode. It 
> probably doesn't do much of a difference.
For "Zero" I usually would just check if the current decl-context is dependent. 
 I'm not positive if that is 'correct' without more thought, though I would 
expect it to be?  I think we make sure that is set up correctly everywhere.  it 
would at least be a good check to see where we mess that up :)



Comment at: clang/test/SemaTemplate/instantiate-var-template.cpp:34
   template void f() {
-static_assert(a == 0, ""); // expected-error 
{{static assertion failed due to requirement 'a == 0'}} \
-   // expected-note 
{{evaluates to '1 == 0'}}
+static_assert(a == 0, ""); // fixme: can we check 
a var is dependant?
   }

cor3ntin wrote:
> erichkeane wrote:
> > You should be able to instantiate this template later, and probably what we 
> > now have to do.  Also, 'dependent' is the spelling in this case, 
> > 'dependant' is something different :)
> I'm afraid doing though would defeat the intent of the test - it is after all 
> named "InstantiationDependent"
Ah, yeah, you're right, it isn't clear what this is trying to test to me 
unfortunately.  It might require some history digging.  Same comment as below 
on the spelling.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144285

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

[PATCH] D144285: [Clang] Implement CWG2518 - static_assert(false)

2023-02-17 Thread Corentin Jabot via Phabricator via cfe-commits 
cor3ntin added a comment.

In D144285#4135775 , @Endill wrote:

> Thank you for the patch.
> Any plans to backport this to 16.x branch?

I think that's something we'd need to discuss with more folks but maybe a bit 
of time to see how this behaves on real code wouldn't hurt - even if I 
understand people have a keen interest in getting access to that sooner rather 
than later.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144285

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

[PATCH] D144285: [Clang] Implement CWG2518 - static_assert(false)

2023-02-17 Thread Corentin Jabot via Phabricator via cfe-commits 
cor3ntin added inline comments.



Comment at: clang/lib/Sema/SemaDeclCXX.cpp:16805
+// definition, the declaration has no effect.
+bool InTemplateDefinition = getLangOpts().CPlusPlus && 
getTemplateDepth(getCurScope()) != 0;
+

erichkeane wrote:
> Hmm... interesting way to calculate template depth, I wasn't aware of that 
> one.  Does this cause problems in 'a template caused another template to 
> instantiate' sorta thing? 
> 
> Also, is the "CPlusPlus" test here necessary?
It's what I came up with. Do you think there is a better way?
If you have suggestions for additional tests, I'm all ears!

The  CplusPlus tests is just there to avoid unnecessary cycles in C mode. It 
probably doesn't do much of a difference.



Comment at: clang/test/SemaTemplate/instantiate-var-template.cpp:34
   template void f() {
-static_assert(a == 0, ""); // expected-error 
{{static assertion failed due to requirement 'a == 0'}} \
-   // expected-note 
{{evaluates to '1 == 0'}}
+static_assert(a == 0, ""); // fixme: can we check 
a var is dependant?
   }

erichkeane wrote:
> You should be able to instantiate this template later, and probably what we 
> now have to do.  Also, 'dependent' is the spelling in this case, 'dependant' 
> is something different :)
I'm afraid doing though would defeat the intent of the test - it is after all 
named "InstantiationDependent"


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144285

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

[PATCH] D144285: [Clang] Implement CWG2518 - static_assert(false)

2023-02-17 Thread Vlad Serebrennikov via Phabricator via cfe-commits 
Endill added a comment.

Thank you for the patch.
Any plans to backport this to 16.x branch?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144285

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

[PATCH] D144285: [Clang] Implement CWG2518 - static_assert(false)

2023-02-17 Thread Erich Keane via Phabricator via cfe-commits 
erichkeane added inline comments.



Comment at: clang/lib/Sema/SemaDeclCXX.cpp:16805
+// definition, the declaration has no effect.
+bool InTemplateDefinition = getLangOpts().CPlusPlus && 
getTemplateDepth(getCurScope()) != 0;
+

Hmm... interesting way to calculate template depth, I wasn't aware of that one. 
 Does this cause problems in 'a template caused another template to 
instantiate' sorta thing? 

Also, is the "CPlusPlus" test here necessary?



Comment at: clang/test/SemaTemplate/instantiate-var-template.cpp:34
   template void f() {
-static_assert(a == 0, ""); // expected-error 
{{static assertion failed due to requirement 'a == 0'}} \
-   // expected-note 
{{evaluates to '1 == 0'}}
+static_assert(a == 0, ""); // fixme: can we check 
a var is dependant?
   }

You should be able to instantiate this template later, and probably what we now 
have to do.  Also, 'dependent' is the spelling in this case, 'dependant' is 
something different :)



Comment at: clang/test/SemaTemplate/instantiation-dependence.cpp:41
 static_assert(!__is_void(indirect_void_t)); // "ok", dependent
-static_assert(!__is_void(void_t)); // expected-error {{failed}}
+static_assert(!__is_void(void_t)); // fixme: can we check a type is 
dependant?
   }

This one is probably a bigger pain to test, and I don't have a good idea.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144285

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

[PATCH] D144285: [Clang] Implement CWG2518 - static_assert(false)

2023-02-17 Thread Corentin Jabot via Phabricator via cfe-commits 
cor3ntin updated this revision to Diff 498448.
cor3ntin added a comment.

Forgot to run clang-format


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144285

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/test/CXX/drs/dr25xx.cpp
  clang/test/SemaCXX/access-base-class.cpp
  clang/test/SemaCXX/coroutines-exp-namespace.cpp
  clang/test/SemaCXX/coroutines.cpp
  clang/test/SemaCXX/static-assert.cpp
  clang/test/SemaTemplate/instantiate-var-template.cpp
  clang/test/SemaTemplate/instantiation-dependence.cpp
  clang/www/cxx_dr_status.html

Index: clang/www/cxx_dr_status.html
===
--- clang/www/cxx_dr_status.html
+++ clang/www/cxx_dr_status.html
@@ -14915,7 +14915,7 @@
 https://wg21.link/cwg2518;>2518
 review
 Conformance requirements and #error/#warning
-Not resolved
+Clang 17
   
   
 https://wg21.link/cwg2519;>2519
Index: clang/test/SemaTemplate/instantiation-dependence.cpp
===
--- clang/test/SemaTemplate/instantiation-dependence.cpp
+++ clang/test/SemaTemplate/instantiation-dependence.cpp
@@ -38,7 +38,7 @@
 
   template void foo() {
 static_assert(!__is_void(indirect_void_t)); // "ok", dependent
-static_assert(!__is_void(void_t)); // expected-error {{failed}}
+static_assert(!__is_void(void_t)); // fixme: can we check a type is dependant?
   }
 }
 
Index: clang/test/SemaTemplate/instantiate-var-template.cpp
===
--- clang/test/SemaTemplate/instantiate-var-template.cpp
+++ clang/test/SemaTemplate/instantiate-var-template.cpp
@@ -31,9 +31,9 @@
   static_assert(b == 1, ""); // expected-note {{in instantiation of}}
 
   template void f() {
-static_assert(a == 0, ""); // expected-error {{static assertion failed due to requirement 'a == 0'}} \
-   // expected-note {{evaluates to '1 == 0'}}
+static_assert(a == 0, ""); // fixme: can we check a var is dependant?
   }
+
 }
 
 namespace PR24483 {
Index: clang/test/SemaCXX/static-assert.cpp
===
--- clang/test/SemaCXX/static-assert.cpp
+++ clang/test/SemaCXX/static-assert.cpp
@@ -54,7 +54,7 @@
   // Only give one error here.
   static_assert(false, ""); // expected-error {{static assertion failed}}
 };
-AlwaysFails alwaysFails;
+AlwaysFails alwaysFails; // expected-note {{instantiation}}
 
 template struct StaticAssertProtected {
   static_assert(__is_literal(T), ""); // expected-error {{static assertion failed}}
Index: clang/test/SemaCXX/coroutines.cpp
===
--- clang/test/SemaCXX/coroutines.cpp
+++ clang/test/SemaCXX/coroutines.cpp
@@ -1309,7 +1309,7 @@
   }
 };
 
-template struct DepTestType; // expected-note {{requested here}}
+template struct DepTestType; // expected-note 2{{requested here}}
 template CoroMemberTag DepTestType::test_member_template(long, const char *) const &&;
 
 template CoroMemberTag DepTestType::test_static_template(const char *volatile &, unsigned);
Index: clang/test/SemaCXX/coroutines-exp-namespace.cpp
===
--- clang/test/SemaCXX/coroutines-exp-namespace.cpp
+++ clang/test/SemaCXX/coroutines-exp-namespace.cpp
@@ -1288,7 +1288,7 @@
   }
 };
 
-template struct DepTestType; // expected-note {{requested here}}
+template struct DepTestType; // expected-note 2{{requested here}}
 template CoroMemberTag DepTestType::test_member_template(long, const char *) const &&;
 
 template CoroMemberTag DepTestType::test_static_template(const char *volatile &, unsigned);
Index: clang/test/SemaCXX/access-base-class.cpp
===
--- clang/test/SemaCXX/access-base-class.cpp
+++ clang/test/SemaCXX/access-base-class.cpp
@@ -96,14 +96,14 @@
 };
 
 template 
-struct trait : flag {};
+struct trait : flag {}; // expected-note 2{{here}}
 
-template ::value>
+template ::value> // expected-note {{here}}
 struct a {};
 
 template 
 class b {
-  a x;
+  a x; // expected-note {{here}}
   using U = a;
 };
 
@@ -113,5 +113,5 @@
 };
 
 // verify "no member named 'value'" bogus diagnostic is not emitted.
-trait>>::value;
+trait>>::value; // expected-note {{here}}
 } // namespace T8
Index: clang/test/CXX/drs/dr25xx.cpp
===
--- clang/test/CXX/drs/dr25xx.cpp
+++ clang/test/CXX/drs/dr25xx.cpp
@@ -1,5 +1,33 @@
 // RUN: %clang_cc1 -std=c++20 -triple x86_64-unknown-unknown %s -verify
 
+namespace dr2518 { // dr2518: 17 review
+
+#error one
+// expected-error@-1 {{one}}
+#if 0
+#error skip
+#warning skip // expected-error {{skip}}
+#endif
+#error two
+// expected-error@-1 {{two}}
+#warning

[PATCH] D144285: [Clang] Implement CWG2518 - static_assert(false)

2023-02-17 Thread Corentin Jabot via Phabricator via cfe-commits 
cor3ntin created this revision.
Herald added a project: All.
cor3ntin requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

This allows `static_assert(false)` to not be ill-formed
in template definitions.

This change is applied as a DR in all C++ modes.

Of notes, a couple of tests were relying of the eager nature
of static_assert

- test/SemaTemplate/instantiation-dependence.cpp
- test/SemaTemplate/instantiate-var-template.cpp

I don't know if the changes to `static_assert`
still allow that sort of tests to be expressed.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D144285

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/test/CXX/drs/dr25xx.cpp
  clang/test/SemaCXX/access-base-class.cpp
  clang/test/SemaCXX/coroutines-exp-namespace.cpp
  clang/test/SemaCXX/coroutines.cpp
  clang/test/SemaCXX/static-assert.cpp
  clang/test/SemaTemplate/instantiate-var-template.cpp
  clang/test/SemaTemplate/instantiation-dependence.cpp
  clang/www/cxx_dr_status.html

Index: clang/www/cxx_dr_status.html
===
--- clang/www/cxx_dr_status.html
+++ clang/www/cxx_dr_status.html
@@ -14915,7 +14915,7 @@
 https://wg21.link/cwg2518;>2518
 review
 Conformance requirements and #error/#warning
-Not resolved
+Clang 17
   
   
 https://wg21.link/cwg2519;>2519
Index: clang/test/SemaTemplate/instantiation-dependence.cpp
===
--- clang/test/SemaTemplate/instantiation-dependence.cpp
+++ clang/test/SemaTemplate/instantiation-dependence.cpp
@@ -38,7 +38,7 @@
 
   template void foo() {
 static_assert(!__is_void(indirect_void_t)); // "ok", dependent
-static_assert(!__is_void(void_t)); // expected-error {{failed}}
+static_assert(!__is_void(void_t)); // fixme: can we check a type is dependant?
   }
 }
 
Index: clang/test/SemaTemplate/instantiate-var-template.cpp
===
--- clang/test/SemaTemplate/instantiate-var-template.cpp
+++ clang/test/SemaTemplate/instantiate-var-template.cpp
@@ -31,9 +31,9 @@
   static_assert(b == 1, ""); // expected-note {{in instantiation of}}
 
   template void f() {
-static_assert(a == 0, ""); // expected-error {{static assertion failed due to requirement 'a == 0'}} \
-   // expected-note {{evaluates to '1 == 0'}}
+static_assert(a == 0, ""); // fixme: can we check a var is dependant?
   }
+
 }
 
 namespace PR24483 {
Index: clang/test/SemaCXX/static-assert.cpp
===
--- clang/test/SemaCXX/static-assert.cpp
+++ clang/test/SemaCXX/static-assert.cpp
@@ -54,7 +54,7 @@
   // Only give one error here.
   static_assert(false, ""); // expected-error {{static assertion failed}}
 };
-AlwaysFails alwaysFails;
+AlwaysFails alwaysFails; // expected-note {{instantiation}}
 
 template struct StaticAssertProtected {
   static_assert(__is_literal(T), ""); // expected-error {{static assertion failed}}
Index: clang/test/SemaCXX/coroutines.cpp
===
--- clang/test/SemaCXX/coroutines.cpp
+++ clang/test/SemaCXX/coroutines.cpp
@@ -1309,7 +1309,7 @@
   }
 };
 
-template struct DepTestType; // expected-note {{requested here}}
+template struct DepTestType; // expected-note 2{{requested here}}
 template CoroMemberTag DepTestType::test_member_template(long, const char *) const &&;
 
 template CoroMemberTag DepTestType::test_static_template(const char *volatile &, unsigned);
Index: clang/test/SemaCXX/coroutines-exp-namespace.cpp
===
--- clang/test/SemaCXX/coroutines-exp-namespace.cpp
+++ clang/test/SemaCXX/coroutines-exp-namespace.cpp
@@ -1288,7 +1288,7 @@
   }
 };
 
-template struct DepTestType; // expected-note {{requested here}}
+template struct DepTestType; // expected-note 2{{requested here}}
 template CoroMemberTag DepTestType::test_member_template(long, const char *) const &&;
 
 template CoroMemberTag DepTestType::test_static_template(const char *volatile &, unsigned);
Index: clang/test/SemaCXX/access-base-class.cpp
===
--- clang/test/SemaCXX/access-base-class.cpp
+++ clang/test/SemaCXX/access-base-class.cpp
@@ -96,14 +96,14 @@
 };
 
 template 
-struct trait : flag {};
+struct trait : flag {}; // expected-note 2{{here}}
 
-template ::value>
+template ::value> // expected-note {{here}}
 struct a {};
 
 template 
 class b {
-  a x;
+  a x; // expected-note {{here}}
   using U = a;
 };
 
@@ -113,5 +113,5 @@
 };
 
 // verify "no member named 'value'" bogus diagnostic is not emitted.
-trait>>::value;
+trait>>::value; // expected-note {{here}}
 } // namespace T8
Index: clang/test/CXX/drs/dr25xx.cpp