[PATCH] D90448: [clang] Add type check for explicit instantiation of static data members

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

rsmith added inline comments.


================
Comment at: clang/lib/Sema/SemaTemplate.cpp:10111-10124
+    // Check the static member's type given in the explicit instantiation
+    // definition against the one in the class template. This won't happen in
+    // explicit instantiation declaration because the instantiated code won't
+    // be generated in that case.
+    if (IsStaticDataMemberInstantiation &&
+        TSK == TSK_ExplicitInstantiationDefinition && Prev &&
+        !Context.hasSameTypeIgnoreLifetime(Prev->getType(), R)) {
----------------
Can we combine this with the previous `if`? This is really checking the same thing that the `hasSameType` check above checked.


================
Comment at: clang/lib/Sema/SemaTemplate.cpp:10116
+    if (IsStaticDataMemberInstantiation &&
+        TSK == TSK_ExplicitInstantiationDefinition && Prev &&
+        !Context.hasSameTypeIgnoreLifetime(Prev->getType(), R)) {
----------------
The check of `TSK` seems unnecessary (and incorrect) here -- we should check the type is correct regardless of whether this is an explicit instantiation declaration or an explicit instantiation definition.


================
Comment at: clang/lib/Sema/SemaTemplate.cpp:10117
+        TSK == TSK_ExplicitInstantiationDefinition && Prev &&
+        !Context.hasSameTypeIgnoreLifetime(Prev->getType(), R)) {
+      Diag(T->getTypeLoc().getBeginLoc(),
----------------
Please can you point us at an example that needs this "ignore lifetime" nuance? We should check with Apple folks what they want to happen here, and we should presumably have the same rule for all explicit instantiations of templated variables -- both in the static data member case here and the variable template case a few lines above.

My expectation is that we want one of these two rules:
1) the declared lifetime should match exactly between the declaration and the explicit instantiation, or
2) there cannot be a declared lifetime on the explicit instantiation, and a lifetime on the template declaration is ignored by the check
(or a combination of these rules where we accept either option). I don't think that matches what you're doing here -- in particular, I think a wrong declared lifetime on the explicit instantiation should result in an error.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90448