[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 always
[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()) { +