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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman 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))
----------------
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 <typename Ty>
> > struct S {
> >   static_assert(false, "you have to use one of the valid specializations, not the primary template");
> > };
> > 
> > template <>
> > struct S<int> {
> > };
> > 
> > template <>
> > struct S<float> {    
> > };
> > 
> > int main() {
> >   S<int> s1;
> >   S<float> s2;
> >   S<double> 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 at -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<double> s; //expected-note{{in instantiation of template class 'DependentAlwaysFalse::S<double>' requested here}}
+  T<double> t; //expected-note{{in instantiation of template class 'DependentAlwaysFalse::T<double>' requested here}}
+}
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144285