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

[Chuyang Chen via Phabricator via cfe-commits]

nomanous marked an inline comment as done.
nomanous 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)) {
----------------
rsmith wrote:
> Can we combine this with the previous `if`? This is really checking the same thing that the `hasSameType` check above checked.
It seems difficult to combine this with the previous one.

  # The two `if` blocks are small and both only contain two statements, but the two statements contained by the posterior one are all different from statements contained by the previous one (they use different diagnostics);
  # The conditions are different. The previous one uses `PrevTemplate` to make sure that it's in a variable template declaration and the posterior one uses `IsStaticDataMemberInstantiation` to make sure that it's in a static data member instantiation of a template class.

So, if we want to combine the two blocks, we must write two sub-`if`-blocks in the combined block, which, I think, has no advantage over the current code (if not more confusing).








================
Comment at: clang/lib/Sema/SemaTemplate.cpp:10117
+        TSK == TSK_ExplicitInstantiationDefinition && Prev &&
+        !Context.hasSameTypeIgnoreLifetime(Prev->getType(), R)) {
+      Diag(T->getTypeLoc().getBeginLoc(),
----------------
rsmith wrote:
> 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.
I'll do some test and give you a code snippet to trigger the lifetime mismatch when lifetime specifiers are not ignored. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90448