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

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.


================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:16824-16841
       if (InnerCond && isa<ConceptSpecializationExpr>(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<typename T> struct AlwaysFails {
   // Only give one error here.
   static_assert(false, ""); // expected-error {{static assertion failed}}
 };
+AlwaysFails<int> 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<typename T> void f() {
-    static_assert(a<sizeof(sizeof(f(T())))> == 0, ""); // expected-error {{static assertion failed due to requirement 'a<sizeof (sizeof (f(type-parameter-0-0())))> == 0'}} \
-                                                       // expected-note {{evaluates to '1 == 0'}}
+    static_assert(a<sizeof(sizeof(f(T())))> == 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<sizeof(sizeof(f(T())))> == 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<T>)); // "ok", dependent
-    static_assert(!__is_void(void_t<T>)); // expected-error {{failed}}
+    static_assert(!__is_void(void_t<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