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

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In D144285#4135775 <https://reviews.llvm.org/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<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?
   }
----------------
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