Re: [PATCH] D24508: PR28752: Do not instantiate var decls which are not visible.
rsmith added a comment.
I think you'll also need to update the `ASTDeclReader` to merge `VarDecl`
definitions together if it reads a definition and there already is one in the
AST. I note that right now `Sema::AddInitializerToDecl` permits multiple
definitions of a `VarDecl`, which doesn't seem like what we want here; we'll
probably want to merge those as we parse them, too.
Now, we have a problem here: unlike with classes and functions, we can't always
convert a variable definition to a declaration by just dropping the
initializer. Perhaps we can add a flag to `VarDecl` to indicate "this is just a
declaration, even though it looks like a definition", to handle that case?
(This would also be useful for `VarTemplateSpecializationDecl`s, where we
currently reject valid code such as "template int n; int k = n;"
because we have no way to represent the difference between a mere declaration
and a definition of `n`.)
Comment at: lib/Sema/SemaTemplate.cpp:505
@@ -499,3 +504,3 @@
Instantiation->setInvalidDecl();
} else if (InstantiatedFromMember) {
if (isa(Instantiation)) {
Why do we not issue a diagnostic in this case for a `VarDecl` when `Complain`
is true and no definition is available? It seems like we should either be
diagnosing this or asserting that it can't happen.
Comment at: lib/Sema/SemaTemplate.cpp:528
@@ +527,3 @@
+ Note = diag::note_template_decl_here;
+} else if (isa(Instantiation)) {
+ if (isa(Instantiation)) {
`else` + `assert` would make more sense here. No other kind of declaration
should get here.
Comment at: lib/Sema/SemaType.cpp:6898-6899
@@ +6897,4 @@
+ } else if (auto *VD = dyn_cast(D)) {
+// FIXME: Handle the case where D is a VarTemplateSpecializationDecl, i.e.
D
+// is not a static data member.
+if (auto *Pattern = VD->getInstantiatedFromStaticDataMember())
We should add a `getTemplateInstantiationPattern()` to `VarDecl` and use it
from here.
https://reviews.llvm.org/D24508
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24508: PR28752: Do not instantiate var decls which are not visible.
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(Instantiation) || isa(Instantiation)
+ || isa(Instantiation));
`||` on the previous line, please.
Comment at: lib/Sema/SemaTemplate.cpp:472
@@ -470,3 +471,3 @@
- if (PatternDef && (isa(PatternDef)
- || !cast(PatternDef)->isBeingDefined())) {
+ bool isEntityBeingDefined = false;
+ if (const TagDecl *TD = dyn_cast_or_null(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(PatternDef),
,
/*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(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(Instantiation)) {
+ if (isa(Instantiation)) {
Likewise here.
Repository:
rL LLVM
https://reviews.llvm.org/D24508
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
