[PATCH] D24508: PR28752: Do not instantiate var decls which are not visible.
rsmith added a comment.
I reverted this in r284081, and relanded with fixes described here as r284284.
Comment at: lib/Sema/SemaDecl.cpp:9712
+
+ // Demote the newly parsed definition to a fake declaration.
+ if (!VDecl->isThisDeclarationADemotedDefinition())
We also need to do this work when MergeVarDecls encounters the same condition.
Comment at: lib/Serialization/ASTReaderDecl.cpp:3087
+ CurD->isThisDeclarationADefinition()) {
+VD->demoteThisDefinitionToDeclaration();
+break;
When we do this, we need to tell the ASTContext we merged the definitions, so
that the old definition will be made visible whenever the new one is.
https://reviews.llvm.org/D24508
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D24508: PR28752: Do not instantiate var decls which are not visible.
v.g.vassilev closed this revision. v.g.vassilev marked an inline comment as done. v.g.vassilev added a comment. Relanded in r284008. https://reviews.llvm.org/D24508 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D24508: PR28752: Do not instantiate var decls which are not visible.
v.g.vassilev updated this revision to Diff 74353.
v.g.vassilev marked 2 inline comments as done.
v.g.vassilev added a comment.
We should not access the IsThisDeclarationADemotedDefinition bits if the decl
is ParmVarDecl.
https://reviews.llvm.org/D24508
Files:
include/clang/AST/Decl.h
lib/AST/Decl.cpp
lib/Sema/SemaDecl.cpp
lib/Sema/SemaTemplate.cpp
lib/Sema/SemaTemplateInstantiateDecl.cpp
lib/Sema/SemaType.cpp
lib/Serialization/ASTReaderDecl.cpp
lib/Serialization/ASTWriterDecl.cpp
test/Modules/Inputs/PR28752/Subdir1/b.h
test/Modules/Inputs/PR28752/Subdir1/c.h
test/Modules/Inputs/PR28752/Subdir1/module.modulemap
test/Modules/Inputs/PR28752/a.h
test/Modules/Inputs/PR28752/module.modulemap
test/Modules/Inputs/PR28752/vector
test/Modules/pr28752.cpp
Index: test/Modules/pr28752.cpp
===
--- /dev/null
+++ test/Modules/pr28752.cpp
@@ -0,0 +1,19 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -std=c++11 -nostdsysteminc -I%S/Inputs/PR28752 -verify %s
+// RUN: %clang_cc1 -std=c++11 -nostdsysteminc -fmodules -fmodule-map-file=%S/Inputs/PR28752/Subdir1/module.modulemap -fmodule-map-file=%S/Inputs/PR28752/module.modulemap -fmodules-cache-path=%t -I%S/Inputs/PR28752 -I%S/Inputs/PR28752/Subdir1 -verify %s
+
+#include "a.h"
+#include "Subdir1/c.h"
+#include
+
+class TClingClassInfo {
+ std::vector fIterStack;
+};
+
+TClingClassInfo *a;
+class TClingBaseClassInfo {
+ TClingBaseClassInfo() { new TClingClassInfo(*a); }
+};
+
+// expected-no-diagnostics
+
Index: test/Modules/Inputs/PR28752/vector
===
--- /dev/null
+++ test/Modules/Inputs/PR28752/vector
@@ -0,0 +1,28 @@
+#ifndef VECTOR
+#define VECTOR
+template struct B;
+template struct B { typedef _Tp type; };
+namespace std {
+template struct D {
+
+ template struct F {
+static const bool value = 0;
+ };
+
+ template
+ typename B::value, _Alloc2>::type _S_select(_Alloc2);
+ template
+ static
+ typename B::value, _Alloc2>::type _S_select(_Alloc2);
+};
+template
+template
+const bool D<_Alloc>::F<_Alloc2>::value;
+
+template class vector {
+public:
+ vector(int);
+ vector(vector &) : vector(D::_S_select((bool)0)) {}
+};
+}
+#endif // VECTOR
\ No newline at end of file
Index: test/Modules/Inputs/PR28752/module.modulemap
===
--- /dev/null
+++ test/Modules/Inputs/PR28752/module.modulemap
@@ -0,0 +1 @@
+module a { header "a.h" export * }
Index: test/Modules/Inputs/PR28752/a.h
===
--- /dev/null
+++ test/Modules/Inputs/PR28752/a.h
@@ -0,0 +1 @@
+#include
Index: test/Modules/Inputs/PR28752/Subdir1/module.modulemap
===
--- /dev/null
+++ test/Modules/Inputs/PR28752/Subdir1/module.modulemap
@@ -0,0 +1,5 @@
+module b {
+ module "b.h" { header "b.h" export * }
+ module "c.h" { header "c.h" export * }
+ export *
+}
Index: test/Modules/Inputs/PR28752/Subdir1/b.h
===
--- /dev/null
+++ test/Modules/Inputs/PR28752/Subdir1/b.h
@@ -0,0 +1 @@
+#include
Index: lib/Serialization/ASTWriterDecl.cpp
===
--- lib/Serialization/ASTWriterDecl.cpp
+++ lib/Serialization/ASTWriterDecl.cpp
@@ -894,6 +894,7 @@
Record.push_back(D->getTSCSpec());
Record.push_back(D->getInitStyle());
if (!isa(D)) {
+Record.push_back(D->isThisDeclarationADemotedDefinition());
Record.push_back(D->isExceptionVariable());
Record.push_back(D->isNRVOVariable());
Record.push_back(D->isCXXForRangeDecl());
@@ -998,6 +999,8 @@
// Check things we know are true of *every* PARM_VAR_DECL, which is more than
// just us assuming it.
assert(!D->getTSCSpec() && "PARM_VAR_DECL can't use TLS");
+ assert(!D->isThisDeclarationADemotedDefinition()
+ && "PARM_VAR_DECL can't be demoted definition.");
assert(D->getAccess() == AS_none && "PARM_VAR_DECL can't be public/private");
assert(!D->isExceptionVariable() && "PARM_VAR_DECL can't be exception var");
assert(D->getPreviousDecl() == nullptr && "PARM_VAR_DECL can't be redecl");
@@ -1957,6 +1960,7 @@
Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 3)); // SClass
Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 2)); // TSCSpec
Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 2)); // InitStyle
+ Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // IsThisDeclarationADemotedDefinition
Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // isExceptionVariable
Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // isNRVOVariable
Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // isCXXForRangeDecl
Index: lib/Serialization/ASTReaderDecl.cpp
[PATCH] D24508: PR28752: Do not instantiate var decls which are not visible.
rsmith added inline comments.
Comment at: include/clang/AST/Decl.h:1213
+ bool isThisDeclarationADemotedDefinition() const {
+return NonParmVarDeclBits.IsThisDeclarationADemotedDefinition;
+ }
This is the bug. You can't read this bit here without first checking whether
you are a ParmVarDecl, because ParmVarDecls don't have this bit at all.
Comment at: include/clang/AST/Decl.h:1222
+ void demoteThisDefinitionToDeclaration() {
+assert (!isThisDeclarationADemotedDefinition() && "Aleady demoted!");
+assert (isThisDeclarationADefinition() && "Not a definition!");
v.g.vassilev wrote:
> rsmith wrote:
> > You can remove this; it's covered by the next line.
> Ok. This would allow calling the method on already demoted definition. Do we
> want to allow that?
Demoted definitions return false from `isThisDeclarationADefinition()`, so the
next line catches this case.
https://reviews.llvm.org/D24508
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D24508: PR28752: Do not instantiate var decls which are not visible.
v.g.vassilev marked 2 inline comments as done.
v.g.vassilev added a comment.
Landed in r283882.
Comment at: include/clang/AST/Decl.h:1222
+ void demoteThisDefinitionToDeclaration() {
+assert (!isThisDeclarationADemotedDefinition() && "Aleady demoted!");
+assert (isThisDeclarationADefinition() && "Not a definition!");
rsmith wrote:
> You can remove this; it's covered by the next line.
Ok. This would allow calling the method on already demoted definition. Do we
want to allow that?
Comment at: lib/AST/Decl.cpp:2284
+ while (auto *NewVD = VD->getInstantiatedFromStaticDataMember())
+VD = NewVD;
+ return VD->getDefinition();
rsmith wrote:
> Missing a member specialization check here.
Isn't that covered by the cases above? It seems there is no API to make such
check, here. The current state looks to me consistent with
`CXXRecordDecl::getTemplateInstantiationPattern` and
`FunctionDecl::getTemplateInstantiationPattern`.
Comment at: lib/Serialization/ASTReaderDecl.cpp:3086-3090
+ if (CurD->isThisDeclarationADemotedDefinition()) {
+VD->demoteThisDefinitionToDeclaration();
+break;
+ }
+ if (CurD->isThisDeclarationADefinition()) {
rsmith wrote:
> Maybe combine these two `if`s into one, since their bodies are identical?
Good point ;)
https://reviews.llvm.org/D24508
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D24508: PR28752: Do not instantiate var decls which are not visible.
rsmith accepted this revision.
rsmith added inline comments.
This revision is now accepted and ready to land.
Comment at: include/clang/AST/Decl.h:1222
+ void demoteThisDefinitionToDeclaration() {
+assert (!isThisDeclarationADemotedDefinition() && "Aleady demoted!");
+assert (isThisDeclarationADefinition() && "Not a definition!");
You can remove this; it's covered by the next line.
Comment at: lib/AST/Decl.cpp:2284
+ while (auto *NewVD = VD->getInstantiatedFromStaticDataMember())
+VD = NewVD;
+ return VD->getDefinition();
Missing a member specialization check here.
Comment at: lib/Serialization/ASTReaderDecl.cpp:3086-3090
+ if (CurD->isThisDeclarationADemotedDefinition()) {
+VD->demoteThisDefinitionToDeclaration();
+break;
+ }
+ if (CurD->isThisDeclarationADefinition()) {
Maybe combine these two `if`s into one, since their bodies are identical?
https://reviews.llvm.org/D24508
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D24508: PR28752: Do not instantiate var decls which are not visible.
v.g.vassilev updated this revision to Diff 74129.
v.g.vassilev marked 3 inline comments as done.
v.g.vassilev added a comment.
Address comments.
https://reviews.llvm.org/D24508
Files:
include/clang/AST/Decl.h
lib/AST/Decl.cpp
lib/Sema/SemaDecl.cpp
lib/Sema/SemaTemplate.cpp
lib/Sema/SemaTemplateInstantiateDecl.cpp
lib/Sema/SemaType.cpp
lib/Serialization/ASTReaderDecl.cpp
lib/Serialization/ASTWriterDecl.cpp
test/Modules/Inputs/PR28752/Subdir1/b.h
test/Modules/Inputs/PR28752/Subdir1/c.h
test/Modules/Inputs/PR28752/Subdir1/module.modulemap
test/Modules/Inputs/PR28752/a.h
test/Modules/Inputs/PR28752/module.modulemap
test/Modules/Inputs/PR28752/vector
test/Modules/pr28752.cpp
Index: test/Modules/pr28752.cpp
===
--- /dev/null
+++ test/Modules/pr28752.cpp
@@ -0,0 +1,19 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -std=c++11 -nostdsysteminc -I%S/Inputs/PR28752 -verify %s
+// RUN: %clang_cc1 -std=c++11 -nostdsysteminc -fmodules -fmodule-map-file=%S/Inputs/PR28752/Subdir1/module.modulemap -fmodule-map-file=%S/Inputs/PR28752/module.modulemap -fmodules-cache-path=%t -I%S/Inputs/PR28752 -I%S/Inputs/PR28752/Subdir1 -verify %s
+
+#include "a.h"
+#include "Subdir1/c.h"
+#include
+
+class TClingClassInfo {
+ std::vector fIterStack;
+};
+
+TClingClassInfo *a;
+class TClingBaseClassInfo {
+ TClingBaseClassInfo() { new TClingClassInfo(*a); }
+};
+
+// expected-no-diagnostics
+
Index: test/Modules/Inputs/PR28752/vector
===
--- /dev/null
+++ test/Modules/Inputs/PR28752/vector
@@ -0,0 +1,28 @@
+#ifndef VECTOR
+#define VECTOR
+template struct B;
+template struct B { typedef _Tp type; };
+namespace std {
+template struct D {
+
+ template struct F {
+static const bool value = 0;
+ };
+
+ template
+ typename B::value, _Alloc2>::type _S_select(_Alloc2);
+ template
+ static
+ typename B::value, _Alloc2>::type _S_select(_Alloc2);
+};
+template
+template
+const bool D<_Alloc>::F<_Alloc2>::value;
+
+template class vector {
+public:
+ vector(int);
+ vector(vector &) : vector(D::_S_select((bool)0)) {}
+};
+}
+#endif // VECTOR
\ No newline at end of file
Index: test/Modules/Inputs/PR28752/module.modulemap
===
--- /dev/null
+++ test/Modules/Inputs/PR28752/module.modulemap
@@ -0,0 +1 @@
+module a { header "a.h" export * }
Index: test/Modules/Inputs/PR28752/a.h
===
--- /dev/null
+++ test/Modules/Inputs/PR28752/a.h
@@ -0,0 +1 @@
+#include
Index: test/Modules/Inputs/PR28752/Subdir1/module.modulemap
===
--- /dev/null
+++ test/Modules/Inputs/PR28752/Subdir1/module.modulemap
@@ -0,0 +1,5 @@
+module b {
+ module "b.h" { header "b.h" export * }
+ module "c.h" { header "c.h" export * }
+ export *
+}
Index: test/Modules/Inputs/PR28752/Subdir1/b.h
===
--- /dev/null
+++ test/Modules/Inputs/PR28752/Subdir1/b.h
@@ -0,0 +1 @@
+#include
Index: lib/Serialization/ASTWriterDecl.cpp
===
--- lib/Serialization/ASTWriterDecl.cpp
+++ lib/Serialization/ASTWriterDecl.cpp
@@ -894,6 +894,7 @@
Record.push_back(D->getTSCSpec());
Record.push_back(D->getInitStyle());
if (!isa(D)) {
+Record.push_back(D->isThisDeclarationADemotedDefinition());
Record.push_back(D->isExceptionVariable());
Record.push_back(D->isNRVOVariable());
Record.push_back(D->isCXXForRangeDecl());
@@ -998,6 +999,8 @@
// Check things we know are true of *every* PARM_VAR_DECL, which is more than
// just us assuming it.
assert(!D->getTSCSpec() && "PARM_VAR_DECL can't use TLS");
+ assert(!D->isThisDeclarationADemotedDefinition()
+ && "PARM_VAR_DECL can't be demoted definition.");
assert(D->getAccess() == AS_none && "PARM_VAR_DECL can't be public/private");
assert(!D->isExceptionVariable() && "PARM_VAR_DECL can't be exception var");
assert(D->getPreviousDecl() == nullptr && "PARM_VAR_DECL can't be redecl");
@@ -1957,6 +1960,7 @@
Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 3)); // SClass
Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 2)); // TSCSpec
Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 2)); // InitStyle
+ Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // IsThisDeclarationADemotedDefinition
Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // isExceptionVariable
Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // isNRVOVariable
Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // isCXXForRangeDecl
Index: lib/Serialization/ASTReaderDecl.cpp
===
--- lib/Serialization/ASTReaderDecl.cpp
+++
[PATCH] D24508: PR28752: Do not instantiate var decls which are not visible.
rsmith added inline comments.
> SemaDecl.cpp:3692-3693
> + // Demote the newly parsed definition to a fake declaration.
> + // FIXME: Sema::AddInitializationToDecl still allows two definitions,
> + // which make the AST variants inconsistent.
> + assert (Def != New && "There is only one definition!");
We always get here after calling that, don't we? So we only briefly have two
definitions, between the point at which we attach the definition and now. If
that's what you're referring to here, this comment could be clearer about it.
> SemaDecl.cpp:3695
> + assert (Def != New && "There is only one definition!");
> + New->demoteThisDefinitionToDeclaration();
> } else if (Old->isStaticDataMember() &&
You should also call `makeMergedDefinitionVisible(Def, New->getLocation())`
here (and also call it for `Def`'s enclosing template if this is a variable
template definition) to make the prior definition visible.
> ASTReaderDecl.cpp:3083-3087
> +// Fast track.
> +if (PrevVD->isThisDeclarationADefinition()) {
> + VD->demoteThisDefinitionToDeclaration();
> + return;
> +}
I don't think this is worthwhile, since it's the first thing the loop below
will check.
> ASTReaderDecl.cpp:3089
> +
> +for (VarDecl *CurD = D->First; CurD; CurD = CurD->getPreviousDecl())
> + if (CurD->isThisDeclarationADefinition()) {
You should start at `PrevVD` not at `D->First`. This loop will currently only
iterate once.
> ASTReaderDecl.cpp:3090
> +for (VarDecl *CurD = D->First; CurD; CurD = CurD->getPreviousDecl())
> + if (CurD->isThisDeclarationADefinition()) {
> +// If we found another definition on the chain, demote the current
> one.
You can also bail out early and demote the current definition if you reach a
previous demoted definition. That would reduce this from quadratic-time to
linear-time.
> v.g.vassilev wrote in ASTWriterDecl.cpp:896-897
> I thought we might need this for c-style `void f(struct S arg)`-like
> constructs where we might need to demote if we merge ParmVarDecls.
We'll still have only one `ParmVarDecl` per `FunctionDecl`, and no-one cares
whether a `ParmVarDecl` is a definition. Also, you assert that the flag is
always false in this case below.
https://reviews.llvm.org/D24508
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D24508: PR28752: Do not instantiate var decls which are not visible.
v.g.vassilev added inline comments. > rsmith wrote in SemaTemplate.cpp:509 > This function still appears to be able to return true (indicating to the > caller that a diagnostic was produced) without actually producing a > diagnostic. Is it better now? > rsmith wrote in SemaTemplate.cpp:505 > 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. I believe it is not implemented, i.e. we didn't have this diagnostics when `VarDecl::getInstantiatedFromStaticDataMember` is true. I added a FIXME: for a future enhancement. > rsmith wrote in ASTWriterDecl.cpp:896-897 > Sink this flag into the "not for `ParmVarDecl`" block below. I thought we might need this for c-style `void f(struct S arg)`-like constructs where we might need to demote if we merge ParmVarDecls. > rsmith wrote in ASTWriterDecl.cpp:1965 > Hmm. The width of the `InitStyle` field is definitely wrong right now, but > should be fixed separately from this change. It looks like we don't hit this > today because we don't use this abbreviation for a variable with an > initializer. In addition to fixing the width of this field, we should also > remove the `getInit() == nullptr` check when selecting the abbreviation in > `ASTDeclWriter::VisitVarDecl`. Committed in r283444. https://reviews.llvm.org/D24508 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D24508: PR28752: Do not instantiate var decls which are not visible.
v.g.vassilev updated this revision to Diff 73803.
v.g.vassilev marked 4 inline comments as done.
v.g.vassilev added a comment.
Rebase, comments some more fixes.
https://reviews.llvm.org/D24508
Files:
include/clang/AST/Decl.h
lib/AST/Decl.cpp
lib/Sema/SemaDecl.cpp
lib/Sema/SemaTemplate.cpp
lib/Sema/SemaTemplateInstantiateDecl.cpp
lib/Sema/SemaType.cpp
lib/Serialization/ASTReaderDecl.cpp
lib/Serialization/ASTWriterDecl.cpp
test/Modules/Inputs/PR28752/Subdir1/b.h
test/Modules/Inputs/PR28752/Subdir1/c.h
test/Modules/Inputs/PR28752/Subdir1/module.modulemap
test/Modules/Inputs/PR28752/a.h
test/Modules/Inputs/PR28752/module.modulemap
test/Modules/Inputs/PR28752/vector
test/Modules/Inputs/merge-class-definition-visibility/a.h
test/Modules/Inputs/merge-class-definition-visibility/b.h
test/Modules/Inputs/merge-class-definition-visibility/c.h
test/Modules/Inputs/merge-class-definition-visibility/d.h
test/Modules/Inputs/merge-class-definition-visibility/e.h
test/Modules/merge-class-definition-visibility.cpp
test/Modules/pr28752.cpp
Index: test/Modules/pr28752.cpp
===
--- /dev/null
+++ test/Modules/pr28752.cpp
@@ -0,0 +1,19 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -std=c++11 -nostdsysteminc -I%S/Inputs/PR28752 -verify %s
+// RUN: %clang_cc1 -std=c++11 -nostdsysteminc -fmodules -fmodule-map-file=%S/Inputs/PR28752/Subdir1/module.modulemap -fmodule-map-file=%S/Inputs/PR28752/module.modulemap -fmodules-cache-path=%t -I%S/Inputs/PR28752 -I%S/Inputs/PR28752/Subdir1 -verify %s
+
+#include "a.h"
+#include "Subdir1/c.h"
+#include
+
+class TClingClassInfo {
+ std::vector fIterStack;
+};
+
+TClingClassInfo *a;
+class TClingBaseClassInfo {
+ TClingBaseClassInfo() { new TClingClassInfo(*a); }
+};
+
+// expected-no-diagnostics
+
Index: test/Modules/merge-class-definition-visibility.cpp
===
--- test/Modules/merge-class-definition-visibility.cpp
+++ test/Modules/merge-class-definition-visibility.cpp
@@ -1,6 +1,6 @@
// RUN: rm -rf %t
// RUN: %clang_cc1 -fmodules -fmodule-map-file=%S/Inputs/merge-class-definition-visibility/modmap \
-// RUN:-I%S/Inputs/merge-class-definition-visibility \
+// RUN:-std=c++1z -I%S/Inputs/merge-class-definition-visibility \
// RUN:-fmodules-cache-path=%t %s -verify \
// RUN:-fmodules-local-submodule-visibility
// expected-no-diagnostics
@@ -10,6 +10,9 @@
typedef X XA;
struct B;
+template int n;
+
+
#include "e.h"
// Ensure that this triggers the import of the second definition from e.h,
// which is necessary to make the definition of A visible in the template
@@ -21,3 +24,5 @@
// and that definition was merged into the canonical definition from b.h,
// so that becomes visible, and we have a visible definition.
B b;
+
+void f (int dflt = n) {}
Index: test/Modules/Inputs/merge-class-definition-visibility/e.h
===
--- test/Modules/Inputs/merge-class-definition-visibility/e.h
+++ test/Modules/Inputs/merge-class-definition-visibility/e.h
@@ -1,3 +1,5 @@
#include "a.h"
struct B {};
+
+template int n;
Index: test/Modules/Inputs/merge-class-definition-visibility/d.h
===
--- test/Modules/Inputs/merge-class-definition-visibility/d.h
+++ test/Modules/Inputs/merge-class-definition-visibility/d.h
@@ -1 +1,3 @@
struct B {};
+
+template int n;
Index: test/Modules/Inputs/merge-class-definition-visibility/c.h
===
--- test/Modules/Inputs/merge-class-definition-visibility/c.h
+++ test/Modules/Inputs/merge-class-definition-visibility/c.h
@@ -1 +1,3 @@
struct A;
+
+inline int var_A;
Index: test/Modules/Inputs/merge-class-definition-visibility/b.h
===
--- test/Modules/Inputs/merge-class-definition-visibility/b.h
+++ test/Modules/Inputs/merge-class-definition-visibility/b.h
@@ -2,3 +2,6 @@
#include "a.h"
struct B {};
+
+template int n;
+int k = n;
Index: test/Modules/Inputs/merge-class-definition-visibility/a.h
===
--- test/Modules/Inputs/merge-class-definition-visibility/a.h
+++ test/Modules/Inputs/merge-class-definition-visibility/a.h
@@ -1 +1,3 @@
struct A {};
+
+inline int var_A = 2;
Index: test/Modules/Inputs/PR28752/vector
===
--- /dev/null
+++ test/Modules/Inputs/PR28752/vector
@@ -0,0 +1,28 @@
+#ifndef VECTOR
+#define VECTOR
+template struct B;
+template struct B { typedef _Tp type; };
+namespace std {
+template struct D {
+
+ template struct F {
+static const bool value = 0;
+ };
+
+ template
+ typename B::value, _Alloc2>::type
[PATCH] D24508: PR28752: Do not instantiate var decls which are not visible.
rsmith added a comment.
This looks like it's going in the right direction.
> Decl.cpp:2269-2272
> + // If we have hit a point where the user provided a specialization of
> + // this template, we're done looking.
> + if (VarTemplate->isMemberSpecialization())
> +break;
I think we need a similar check in the static data member case above.
> Decl.cpp:2278
> +!isTemplateInstantiation(getTemplateSpecializationKind())) &&
> + "couldn't find pattern for enum instantiation");
> +
enum?
> rsmith wrote in SemaTemplate.cpp:509
> `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.
This function still appears to be able to return true (indicating to the caller
that a diagnostic was produced) without actually producing a diagnostic.
> ASTWriterDecl.cpp:896-897
>Record.push_back(D->getInitStyle());
> + Record.push_back(D->isThisDeclarationADemotedDefinition());
>if (!isa(D)) {
> Record.push_back(D->isExceptionVariable());
Sink this flag into the "not for `ParmVarDecl`" block below.
> ASTWriterDecl.cpp:1965
> + Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 2)); // TSCSpec
> + Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 2)); // InitStyle
> + Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); //
> IsThisDeclarationADemotedDefinition
Hmm. The width of the `InitStyle` field is definitely wrong right now, but
should be fixed separately from this change. It looks like we don't hit this
today because we don't use this abbreviation for a variable with an
initializer. In addition to fixing the width of this field, we should also
remove the `getInit() == nullptr` check when selecting the abbreviation in
`ASTDeclWriter::VisitVarDecl`.
https://reviews.llvm.org/D24508
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D24508: PR28752: Do not instantiate var decls which are not visible.
v.g.vassilev updated this revision to Diff 73688.
v.g.vassilev added a comment.
Address some comments and publish current progress.
https://reviews.llvm.org/D24508
Files:
include/clang/AST/Decl.h
lib/AST/Decl.cpp
lib/Sema/SemaTemplate.cpp
lib/Sema/SemaTemplateInstantiateDecl.cpp
lib/Sema/SemaType.cpp
lib/Serialization/ASTReaderDecl.cpp
lib/Serialization/ASTWriterDecl.cpp
test/Modules/Inputs/PR28752/Subdir1/b.h
test/Modules/Inputs/PR28752/Subdir1/c.h
test/Modules/Inputs/PR28752/Subdir1/module.modulemap
test/Modules/Inputs/PR28752/a.h
test/Modules/Inputs/PR28752/module.modulemap
test/Modules/Inputs/PR28752/vector
test/Modules/Inputs/merge-class-definition-visibility/a.h
test/Modules/Inputs/merge-class-definition-visibility/c.h
test/Modules/merge-class-definition-visibility.cpp
test/Modules/pr28752.cpp
Index: test/Modules/pr28752.cpp
===
--- /dev/null
+++ test/Modules/pr28752.cpp
@@ -0,0 +1,19 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -std=c++11 -nostdsysteminc -I%S/Inputs/PR28752 -verify %s
+// RUN: %clang_cc1 -std=c++11 -nostdsysteminc -fmodules -fmodule-map-file=%S/Inputs/PR28752/Subdir1/module.modulemap -fmodule-map-file=%S/Inputs/PR28752/module.modulemap -fmodules-cache-path=%t -I%S/Inputs/PR28752 -I%S/Inputs/PR28752/Subdir1 -verify %s
+
+#include "a.h"
+#include "Subdir1/c.h"
+#include
+
+class TClingClassInfo {
+ std::vector fIterStack;
+};
+
+TClingClassInfo *a;
+class TClingBaseClassInfo {
+ TClingBaseClassInfo() { new TClingClassInfo(*a); }
+};
+
+// expected-no-diagnostics
+
Index: test/Modules/merge-class-definition-visibility.cpp
===
--- test/Modules/merge-class-definition-visibility.cpp
+++ test/Modules/merge-class-definition-visibility.cpp
@@ -1,6 +1,6 @@
// RUN: rm -rf %t
// RUN: %clang_cc1 -fmodules -fmodule-map-file=%S/Inputs/merge-class-definition-visibility/modmap \
-// RUN:-I%S/Inputs/merge-class-definition-visibility \
+// RUN:-std=c++1z -I%S/Inputs/merge-class-definition-visibility \
// RUN:-fmodules-cache-path=%t %s -verify \
// RUN:-fmodules-local-submodule-visibility
// expected-no-diagnostics
Index: test/Modules/Inputs/merge-class-definition-visibility/c.h
===
--- test/Modules/Inputs/merge-class-definition-visibility/c.h
+++ test/Modules/Inputs/merge-class-definition-visibility/c.h
@@ -1 +1,3 @@
struct A;
+
+inline int var_A;
Index: test/Modules/Inputs/merge-class-definition-visibility/a.h
===
--- test/Modules/Inputs/merge-class-definition-visibility/a.h
+++ test/Modules/Inputs/merge-class-definition-visibility/a.h
@@ -1 +1,3 @@
struct A {};
+
+inline int var_A = 2;
Index: test/Modules/Inputs/PR28752/vector
===
--- /dev/null
+++ test/Modules/Inputs/PR28752/vector
@@ -0,0 +1,28 @@
+#ifndef VECTOR
+#define VECTOR
+template struct B;
+template struct B { typedef _Tp type; };
+namespace std {
+template struct D {
+
+ template struct F {
+static const bool value = 0;
+ };
+
+ template
+ typename B::value, _Alloc2>::type _S_select(_Alloc2);
+ template
+ static
+ typename B::value, _Alloc2>::type _S_select(_Alloc2);
+};
+template
+template
+const bool D<_Alloc>::F<_Alloc2>::value;
+
+template class vector {
+public:
+ vector(int);
+ vector(vector &) : vector(D::_S_select((bool)0)) {}
+};
+}
+#endif // VECTOR
\ No newline at end of file
Index: test/Modules/Inputs/PR28752/module.modulemap
===
--- /dev/null
+++ test/Modules/Inputs/PR28752/module.modulemap
@@ -0,0 +1 @@
+module a { header "a.h" export * }
Index: test/Modules/Inputs/PR28752/a.h
===
--- /dev/null
+++ test/Modules/Inputs/PR28752/a.h
@@ -0,0 +1 @@
+#include
Index: test/Modules/Inputs/PR28752/Subdir1/module.modulemap
===
--- /dev/null
+++ test/Modules/Inputs/PR28752/Subdir1/module.modulemap
@@ -0,0 +1,5 @@
+module b {
+ module "b.h" { header "b.h" export * }
+ module "c.h" { header "c.h" export * }
+ export *
+}
Index: test/Modules/Inputs/PR28752/Subdir1/b.h
===
--- /dev/null
+++ test/Modules/Inputs/PR28752/Subdir1/b.h
@@ -0,0 +1 @@
+#include
Index: lib/Serialization/ASTWriterDecl.cpp
===
--- lib/Serialization/ASTWriterDecl.cpp
+++ lib/Serialization/ASTWriterDecl.cpp
@@ -893,6 +893,7 @@
Record.push_back(D->getStorageClass());
Record.push_back(D->getTSCSpec());
Record.push_back(D->getInitStyle());
+
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
[PATCH] D24508: PR28752: Do not instantiate var decls which are not visible.
v.g.vassilev created this revision.
v.g.vassilev added a reviewer: rsmith.
v.g.vassilev added a subscriber: cfe-commits.
v.g.vassilev set the repository for this revision to rL LLVM.
Repository:
rL LLVM
https://reviews.llvm.org/D24508
Files:
lib/Sema/SemaTemplate.cpp
lib/Sema/SemaTemplateInstantiateDecl.cpp
test/Modules/Inputs/PR28752/Subdir1/b.h
test/Modules/Inputs/PR28752/Subdir1/c.h
test/Modules/Inputs/PR28752/Subdir1/module.modulemap
test/Modules/Inputs/PR28752/a.h
test/Modules/Inputs/PR28752/module.modulemap
test/Modules/Inputs/PR28752/vector
test/Modules/pr28752.cpp
Index: test/Modules/pr28752.cpp
===
--- /dev/null
+++ test/Modules/pr28752.cpp
@@ -0,0 +1,19 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -std=c++11 -nostdsysteminc -I%S/Inputs/PR28752 -verify %s
+// RUN: %clang_cc1 -std=c++11 -nostdsysteminc -fmodules -fmodule-map-file=%S/Inputs/PR28752/Subdir1/module.modulemap -fmodule-map-file=%S/Inputs/PR28752/module.modulemap -fmodules-cache-path=%t -I%S/Inputs/PR28752 -I%S/Inputs/PR28752/Subdir1 -verify %s
+
+#include "a.h"
+#include "Subdir1/c.h"
+#include
+
+class TClingClassInfo {
+ std::vector fIterStack;
+};
+
+TClingClassInfo *a;
+class TClingBaseClassInfo {
+ TClingBaseClassInfo() { new TClingClassInfo(*a); }
+};
+
+// expected-no-diagnostics
+
Index: test/Modules/Inputs/PR28752/vector
===
--- /dev/null
+++ test/Modules/Inputs/PR28752/vector
@@ -0,0 +1,28 @@
+#ifndef VECTOR
+#define VECTOR
+template struct B;
+template struct B { typedef _Tp type; };
+namespace std {
+template struct D {
+
+ template struct F {
+static const bool value = 0;
+ };
+
+ template
+ typename B::value, _Alloc2>::type _S_select(_Alloc2);
+ template
+ static
+ typename B::value, _Alloc2>::type _S_select(_Alloc2);
+};
+template
+template
+const bool D<_Alloc>::F<_Alloc2>::value;
+
+template class vector {
+public:
+ vector(int);
+ vector(vector &) : vector(D::_S_select((bool)0)) {}
+};
+}
+#endif // VECTOR
\ No newline at end of file
Index: test/Modules/Inputs/PR28752/module.modulemap
===
--- /dev/null
+++ test/Modules/Inputs/PR28752/module.modulemap
@@ -0,0 +1 @@
+module a { header "a.h" export * }
Index: test/Modules/Inputs/PR28752/a.h
===
--- /dev/null
+++ test/Modules/Inputs/PR28752/a.h
@@ -0,0 +1 @@
+#include
Index: test/Modules/Inputs/PR28752/Subdir1/module.modulemap
===
--- /dev/null
+++ test/Modules/Inputs/PR28752/Subdir1/module.modulemap
@@ -0,0 +1,5 @@
+module b {
+ module "b.h" { header "b.h" export * }
+ module "c.h" { header "c.h" export * }
+ export *
+}
Index: test/Modules/Inputs/PR28752/Subdir1/b.h
===
--- /dev/null
+++ test/Modules/Inputs/PR28752/Subdir1/b.h
@@ -0,0 +1 @@
+#include
Index: lib/Sema/SemaTemplateInstantiateDecl.cpp
===
--- lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -4067,6 +4067,10 @@
PrettyDeclStackTraceEntry CrashInfo(*this, Var, SourceLocation(),
"instantiating variable initializer");
+ // The instantiation is visible here, even if it was first declared in an
+ // unimported module.
+ Var->setHidden(false);
+
// If we're performing recursive template instantiation, create our own
// queue of pending implicit instantiations that we will instantiate
// later, while we're still within our own instantiation context.
@@ -4115,47 +4119,38 @@
Def = PatternDecl->getDefinition();
}
- // FIXME: Check that the definition is visible before trying to instantiate
- // it. This requires us to track the instantiation stack in order to know
- // which definitions should be visible.
+ TemplateSpecializationKind TSK = Var->getTemplateSpecializationKind();
// If we don't have a definition of the variable template, we won't perform
// any instantiation. Rather, we rely on the user to instantiate this
// definition (or provide a specialization for it) in another translation
// unit.
- if (!Def) {
-if (DefinitionRequired) {
- if (VarSpec)
-Diag(PointOfInstantiation,
- diag::err_explicit_instantiation_undefined_var_template) << Var;
- else
-Diag(PointOfInstantiation,
- diag::err_explicit_instantiation_undefined_member)
-<< 2 << Var->getDeclName() << Var->getDeclContext();
- Diag(PatternDecl->getLocation(),
- diag::note_explicit_instantiation_here);
- if (VarSpec)
-Var->setInvalidDecl();
-} else if
