[PATCH] D41627: [Modules TS] Fix namespace visibility
hamzasood updated this revision to Diff 156689.
hamzasood added a comment.
- Don't assume that -fmodules-ts implies that we're in a TS module.
- Don't ignore private namespaces when calculating the ownership kind.
https://reviews.llvm.org/D41627
Files:
include/clang/AST/DeclBase.h
lib/AST/DeclBase.cpp
lib/Sema/SemaDeclCXX.cpp
test/CXX/modules-ts/basic/basic.namespace/p1.cpp
Index: test/CXX/modules-ts/basic/basic.namespace/p1.cpp
===
--- test/CXX/modules-ts/basic/basic.namespace/p1.cpp
+++ test/CXX/modules-ts/basic/basic.namespace/p1.cpp
@@ -0,0 +1,15 @@
+// RUN: %clang_cc1 -fmodules-ts -DMODULE -emit-module-interface %s -o %t
+// RUN: %clang_cc1 -fmodules-ts -DUSER -fmodule-file=%t %s -verify
+// expected-no-diagnostics
+
+#ifdef MODULE
+export module A;
+namespace ns {
+ export class A { };
+}
+#endif
+
+#ifdef USER
+import A;
+ns::A a;
+#endif
Index: lib/Sema/SemaDeclCXX.cpp
===
--- lib/Sema/SemaDeclCXX.cpp
+++ lib/Sema/SemaDeclCXX.cpp
@@ -8866,6 +8866,21 @@
// for the namespace has the declarations that showed up in that particular
// namespace definition.
PushDeclContext(NamespcScope, Namespc);
+
+ if (const Module *CurrentModule = getCurrentModule()) {
+// Under the Modules TS, a namespace with external linkage should always be
+// visible when imported, regardless of whether it's explicitly exported.
+const bool IsTSModule = CurrentModule->Kind == Module::ModuleInterfaceUnit
||
+CurrentModule->Kind ==
Module::GlobalModuleFragment;
+if (IsTSModule &&
+!Namespc->isAnonymousNamespace() &&
!Namespc->isInAnonymousNamespace()) {
+ Namespc->setModuleOwnershipKind(
+CurrentModule->Kind == Module::ModuleKind::GlobalModuleFragment
+? Decl::ModuleOwnershipKind::Visible
+: Decl::ModuleOwnershipKind::VisibleWhenImported);
+}
+ }
+
return Namespc;
}
Index: lib/AST/DeclBase.cpp
===
--- lib/AST/DeclBase.cpp
+++ lib/AST/DeclBase.cpp
@@ -292,6 +292,23 @@
// Out-of-line virtual method providing a home for Decl.
Decl::~Decl() = default;
+Decl::ModuleOwnershipKind Decl::getModuleOwnershipKindForChildOf(DeclContext
*DC) {
+ // Ignore public namespaces because they might be visible by default.
+ while (DC && DC->isNamespace() && !cast(DC)->isModulePrivate())
+DC = DC->getParent();
+
+ if (auto *D = cast_or_null(DC)) {
+auto MOK = D->getModuleOwnershipKind();
+if (MOK != ModuleOwnershipKind::Unowned &&
+(!D->isFromASTFile() || D->hasLocalOwningModuleStorage()))
+ return MOK;
+// If D is not local and we have no local module storage, then we don't
+// need to track module ownership at all.
+ }
+
+ return ModuleOwnershipKind::Unowned;
+}
+
void Decl::setDeclContext(DeclContext *DC) {
DeclCtx = DC;
}
Index: include/clang/AST/DeclBase.h
===
--- include/clang/AST/DeclBase.h
+++ include/clang/AST/DeclBase.h
@@ -350,18 +350,7 @@
/// Get the module ownership kind to use for a local lexical child of \p DC,
/// which may be either a local or (rarely) an imported declaration.
- static ModuleOwnershipKind getModuleOwnershipKindForChildOf(DeclContext *DC)
{
-if (DC) {
- auto *D = cast(DC);
- auto MOK = D->getModuleOwnershipKind();
- if (MOK != ModuleOwnershipKind::Unowned &&
- (!D->isFromASTFile() || D->hasLocalOwningModuleStorage()))
-return MOK;
- // If D is not local and we have no local module storage, then we don't
- // need to track module ownership at all.
-}
-return ModuleOwnershipKind::Unowned;
- }
+ static ModuleOwnershipKind getModuleOwnershipKindForChildOf(DeclContext *DC);
protected:
Decl(Kind DK, DeclContext *DC, SourceLocation L)
Index: test/CXX/modules-ts/basic/basic.namespace/p1.cpp
===
--- test/CXX/modules-ts/basic/basic.namespace/p1.cpp
+++ test/CXX/modules-ts/basic/basic.namespace/p1.cpp
@@ -0,0 +1,15 @@
+// RUN: %clang_cc1 -fmodules-ts -DMODULE -emit-module-interface %s -o %t
+// RUN: %clang_cc1 -fmodules-ts -DUSER -fmodule-file=%t %s -verify
+// expected-no-diagnostics
+
+#ifdef MODULE
+export module A;
+namespace ns {
+ export class A { };
+}
+#endif
+
+#ifdef USER
+import A;
+ns::A a;
+#endif
Index: lib/Sema/SemaDeclCXX.cpp
===
--- lib/Sema/SemaDeclCXX.cpp
+++ lib/Sema/SemaDeclCXX.cpp
@@ -8866,6 +8866,21 @@
// for the namespace has the declarations that showed up in that particular
// namespace definition.
PushDeclContext(NamespcScope, Namespc);
+
+ if (const Module *CurrentModule = getCurrentModule()) {
+// Under the Modules TS, a namespace with external linkage should
[PATCH] D41627: [Modules TS] Fix namespace visibility
rsmith added inline comments.
Comment at: lib/AST/DeclBase.cpp:285
+Decl::ModuleOwnershipKind Decl::getModuleOwnershipKindForChildOf(DeclContext
*DC) {
+ // Ignore namespaces because they're visible by default.
+ while (DC && DC->isNamespace())
"because they're" -> "because they might be" -- this only applies to Modules TS
modules, not to header modules.
Comment at: lib/Sema/SemaDeclCXX.cpp:8629
+ // regardless of whether or not it's explicitly exported.
+ if (getLangOpts().ModulesTS &&
+ !Namespc->isAnonymousNamespace() && !Namespc->isInAnonymousNamespace()) {
This should depend on the kind of module we're in, not whether `-fmodules-ts`
is enabled.
https://reviews.llvm.org/D41627
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D41627: [Modules TS] Fix namespace visibility
hamzasood created this revision.
hamzasood added reviewers: rsmith, bruno, boris.
Namespaces without an explicit `export` were being mistakenly marked as module
private.
This patch fixes the visibility as per `[basic.namespace]p1` and adds a test
case with previously rejected (yet valid) code.
https://reviews.llvm.org/D41627
Files:
include/clang/AST/DeclBase.h
lib/AST/DeclBase.cpp
lib/Sema/SemaDeclCXX.cpp
test/CXX/modules-ts/basic/basic.namespace/p1.cpp
Index: test/CXX/modules-ts/basic/basic.namespace/p1.cpp
===
--- test/CXX/modules-ts/basic/basic.namespace/p1.cpp
+++ test/CXX/modules-ts/basic/basic.namespace/p1.cpp
@@ -0,0 +1,15 @@
+// RUN: %clang_cc1 -fmodules-ts -DMODULE -emit-module-interface %s -o %t
+// RUN: %clang_cc1 -fmodules-ts -DUSER -fmodule-file=%t %s -verify
+// expected-no-diagnostics
+
+#ifdef MODULE
+export module A;
+namespace ns {
+ export class A { };
+}
+#endif
+
+#ifdef USER
+import A;
+ns::A a;
+#endif
Index: lib/Sema/SemaDeclCXX.cpp
===
--- lib/Sema/SemaDeclCXX.cpp
+++ lib/Sema/SemaDeclCXX.cpp
@@ -8623,6 +8623,19 @@
// for the namespace has the declarations that showed up in that particular
// namespace definition.
PushDeclContext(NamespcScope, Namespc);
+
+ // A namespace with external linkage should always be visible when imported,
+ // regardless of whether or not it's explicitly exported.
+ if (getLangOpts().ModulesTS &&
+ !Namespc->isAnonymousNamespace() && !Namespc->isInAnonymousNamespace()) {
+const Module *CurrentModule = getCurrentModule();
+assert(CurrentModule);
+Namespc->setModuleOwnershipKind(
+CurrentModule->Kind == Module::ModuleKind::GlobalModuleFragment
+? Decl::ModuleOwnershipKind::Visible
+: Decl::ModuleOwnershipKind::VisibleWhenImported);
+ }
+
return Namespc;
}
Index: lib/AST/DeclBase.cpp
===
--- lib/AST/DeclBase.cpp
+++ lib/AST/DeclBase.cpp
@@ -281,6 +281,23 @@
// Out-of-line virtual method providing a home for Decl.
Decl::~Decl() = default;
+Decl::ModuleOwnershipKind Decl::getModuleOwnershipKindForChildOf(DeclContext
*DC) {
+ // Ignore namespaces because they're visible by default.
+ while (DC && DC->isNamespace())
+DC = DC->getParent();
+
+ if (auto *D = cast_or_null(DC)) {
+auto MOK = D->getModuleOwnershipKind();
+if (MOK != ModuleOwnershipKind::Unowned &&
+(!D->isFromASTFile() || D->hasLocalOwningModuleStorage()))
+ return MOK;
+// If D is not local and we have no local module storage, then we don't
+// need to track module ownership at all.
+ }
+
+ return ModuleOwnershipKind::Unowned;
+}
+
void Decl::setDeclContext(DeclContext *DC) {
DeclCtx = DC;
}
Index: include/clang/AST/DeclBase.h
===
--- include/clang/AST/DeclBase.h
+++ include/clang/AST/DeclBase.h
@@ -349,18 +349,7 @@
/// Get the module ownership kind to use for a local lexical child of \p DC,
/// which may be either a local or (rarely) an imported declaration.
- static ModuleOwnershipKind getModuleOwnershipKindForChildOf(DeclContext *DC)
{
-if (DC) {
- auto *D = cast(DC);
- auto MOK = D->getModuleOwnershipKind();
- if (MOK != ModuleOwnershipKind::Unowned &&
- (!D->isFromASTFile() || D->hasLocalOwningModuleStorage()))
-return MOK;
- // If D is not local and we have no local module storage, then we don't
- // need to track module ownership at all.
-}
-return ModuleOwnershipKind::Unowned;
- }
+ static ModuleOwnershipKind getModuleOwnershipKindForChildOf(DeclContext *DC);
protected:
Decl(Kind DK, DeclContext *DC, SourceLocation L)
Index: test/CXX/modules-ts/basic/basic.namespace/p1.cpp
===
--- test/CXX/modules-ts/basic/basic.namespace/p1.cpp
+++ test/CXX/modules-ts/basic/basic.namespace/p1.cpp
@@ -0,0 +1,15 @@
+// RUN: %clang_cc1 -fmodules-ts -DMODULE -emit-module-interface %s -o %t
+// RUN: %clang_cc1 -fmodules-ts -DUSER -fmodule-file=%t %s -verify
+// expected-no-diagnostics
+
+#ifdef MODULE
+export module A;
+namespace ns {
+ export class A { };
+}
+#endif
+
+#ifdef USER
+import A;
+ns::A a;
+#endif
Index: lib/Sema/SemaDeclCXX.cpp
===
--- lib/Sema/SemaDeclCXX.cpp
+++ lib/Sema/SemaDeclCXX.cpp
@@ -8623,6 +8623,19 @@
// for the namespace has the declarations that showed up in that particular
// namespace definition.
PushDeclContext(NamespcScope, Namespc);
+
+ // A namespace with external linkage should always be visible when imported,
+ // regardless of whether or not it's explicitly exported.
+ if (getLangOpts().ModulesTS &&
+ !Namespc->isAnonymousNamespace() && !Namespc->isInAnonymousNamespace()) {
+
