[PATCH] D24508: PR28752: Do not instantiate var decls which are not visible.

[Richard Smith via cfe-commits]

rsmith added a comment.

I expect this patch to cause problems if the two definitions of the variable template come from different modules, because at deserialization time we don't merge the definitions together sensibly (it looks like we end up with a redeclaration chain with multiple declarations, multiple of which believe they are "the" defintiion).


================
Comment at: lib/Sema/SemaTemplate.cpp:470
@@ -470,1 +469,3 @@
+  assert(isa<TagDecl>(Instantiation) || isa<FunctionDecl>(Instantiation)
+         || isa<VarDecl>(Instantiation));
 
----------------
`||` on the previous line, please.

================
Comment at: lib/Sema/SemaTemplate.cpp:472
@@ -470,3 +471,3 @@
 
-  if (PatternDef && (isa<FunctionDecl>(PatternDef)
-                     || !cast<TagDecl>(PatternDef)->isBeingDefined())) {
+  bool isEntityBeingDefined = false;
+  if (const TagDecl *TD = dyn_cast_or_null<TagDecl>(PatternDef))
----------------
Variable names should start with a capital letter.

================
Comment at: lib/Sema/SemaTemplate.cpp:478
@@ -473,3 +477,3 @@
     NamedDecl *SuggestedDef = nullptr;
     if (!hasVisibleDefinition(const_cast<NamedDecl*>(PatternDef), &SuggestedDef,
                               /*OnlyNeedComplete*/false)) {
----------------
We'll need to extend `hasVisibleDefinition` to handle merged `VarDecl`s to support this. (The `ASTReader` doesn't currently merge together `VarDecl` definitions in a reasonable way.)

================
Comment at: lib/Sema/SemaTemplate.cpp:509
@@ -504,3 +508,3 @@
         << 1 << Instantiation->getDeclName() << Instantiation->getDeclContext();
-    } else {
+    } else if (isa<TagDecl>(Instantiation)) {
       Diag(PointOfInstantiation,
----------------
`else if` doesn't make sense here -- we either need to produce a diagnostic on all paths through here, or suppress the notes if we didn't produce a diagnostic.

================
Comment at: lib/Sema/SemaTemplate.cpp:529
@@ +528,3 @@
+      Note = diag::note_template_decl_here;
+    } else if (isa<VarDecl>(Instantiation)) {
+      if (isa<VarTemplateSpecializationDecl>(Instantiation)) {
----------------
Likewise here.


Repository:
  rL LLVM

https://reviews.llvm.org/D24508