[PATCH] D22053: [Sema] Fix a C++1z bug where initializer for static constexpr data member was not instantiated

[Erik Pilkington via cfe-commits]

erik.pilkington added inline comments.

================
Comment at: lib/Sema/SemaTemplateInstantiateDecl.cpp:3861
@@ -3860,3 +3860,3 @@
   // static data members until a definition of the variable is needed. We need
-  // it right away if the type contains 'auto'.
+  // it right away if the type contains 'auto' or is an IncompleteArrayType.
   if ((!isa<VarTemplateSpecializationDecl>(NewVar) &&
----------------
rsmith wrote:
> I don't see why we would need the initializer right away in the `IncompleteArrayType` case. It seems instead that we should delay instantiation of the initializer until a complete type is required for the variable (`Sema::RequireCompleteExprType` handles this case) or it is used in a context that requires a definition (`Sema::MarkVariableReferenced` handles this case).
> 
> 
> You can reproduce a related bug in C++11 mode like so:
> 
>     template<typename T> struct X { static const int arr[]; };
>     template<typename T> constexpr int X<T>::arr[] = {1, 2, 3};
>     constexpr int k = X<int>::arr[0];
> 
> or:
> 
>     template<typename T> struct X { static const double n; };
>     template<typename T> constexpr double X<T>::n = 1;
>     template<int> struct Y {};
>     Y<(int)X<int>::n> y;
> 
> It looks like the bug in this case is that `VarDecl::isUsableInConstantExpressions` (called from `DoMarkVarDeclReferenced`) is mishandling this case: once it's determined that the variable is of non-`volatile` `const`-qualified type, it needs to map back to the template instantiation pattern and check whether the most recent declaration of that is declared `constexpr`, since there might be a not-yet-instantiated redeclaration that adds the `constexpr`.
> 
> 
> I'm not sure that's the same bug you're hitting here, though, since in this case we should be instantiating the `constexpr` specifier with the initial declaration, which should be enough to cause `isUsableInConstantExpressions` to return `true`. So the mystery is, why is `DoMarkVarDeclReferenced` not triggering instantiation?
Thanks for the pointer!

After looking some more into this, it looks like the problem isn't that the initializer for `s_v` isn't instantiated, its that it is instantiated after forming the `DeclRefExpr` to it, this means that the referring expression incorrectly has type `IncompleteArrayType`, which ExprConstant cannot handle. One approach is to patch up the offending `DeclRefExpr` to `s_v` in `DoMarkVarDeclReferenced` to have the correct type after instantiating `s_v`'s initializer, which works fine.

To me, It seems cleaner to eagerly instantiate the initializer (as I did here) so that the `DeclRefExpr` has the correct type right off the bat. This seems very similar to the `auto` case (again, where the exact type isn't known) and is what is done in C++14 mode, FWIW.

If you know the right way to fix this (and have the time/inclination) you should definitely just do it. I would hate to stand in the way of a bug being fixed, especially one that is affecting so many users.

Thanks for helping!


https://reviews.llvm.org/D22053