ChuanqiXu created this revision.
ChuanqiXu added reviewers: iains, rsmith, clang-language-wg.
ChuanqiXu added a project: clang.
Herald added a subscriber: dexonsmith.
Herald added a project: All.
ChuanqiXu requested review of this revision.
Herald added a subscriber: cfe-commits.
Now the implementation would accept following code:
//--- impl.cppm
module M:impl;
class A {};
//--- M.cppm
export module M;
import :impl;
//--- Use.cpp
import M;
void test() {
A a; // Expected error. A is not visible here.
}
which is clearly wrong. The root cause is the implementation of
`isInCurrentModule` would return true if the module is a partition! So in the
above example, although Use.cpp is not a module unit, `isInCurrentModule `
would still return true when the compiler tries to see if the owning module of
`A` is the current module. I believe this is an oversight. And we could fix it
simply by comparing the primary module name.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D123837
Files:
clang/include/clang/Basic/Module.h
clang/lib/Sema/SemaLookup.cpp
clang/test/CXX/module/module.import/p2.cpp
Index: clang/test/CXX/module/module.import/p2.cpp
===================================================================
--- /dev/null
+++ clang/test/CXX/module/module.import/p2.cpp
@@ -0,0 +1,21 @@
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: split-file %s %t
+// RUN: %clang_cc1 -std=c++20 %t/impl.cppm -emit-module-interface -o
%t/M-impl.pcm
+// RUN: %clang_cc1 -std=c++20 %t/M.cppm -emit-module-interface
-fprebuilt-module-path=%t -o %t/M.pcm
+// RUN: %clang_cc1 -std=c++20 %t/Use.cpp -fprebuilt-module-path=%t -verify
-fsyntax-only
+
+//--- impl.cppm
+module M:impl;
+class A {};
+
+//--- M.cppm
+export module M;
+import :impl;
+export A f();
+
+//--- Use.cpp
+import M;
+void test() {
+ A a; // expected-error {{unknown type name 'A'}}
+}
Index: clang/lib/Sema/SemaLookup.cpp
===================================================================
--- clang/lib/Sema/SemaLookup.cpp
+++ clang/lib/Sema/SemaLookup.cpp
@@ -1561,12 +1561,11 @@
static bool isInCurrentModule(const Module *M, const LangOptions &LangOpts) {
// If M is the global module fragment of a module that we've not yet finished
// parsing, then it must be part of the current module.
- // If it's a partition, then it must be visible to an importer (since only
- // another partition or the named module can import it).
- return M->getTopLevelModuleName() == LangOpts.CurrentModule ||
- (M->Kind == Module::GlobalModuleFragment && !M->Parent) ||
- M->Kind == Module::ModulePartitionInterface ||
- M->Kind == Module::ModulePartitionImplementation;
+ if (M->isGlobalModule() && !M->Parent)
+ return true;
+
+ return M->getPrimaryModuleInterfaceName() ==
+ Module::getPrimaryModuleInterfaceName(LangOpts.CurrentModule);
}
bool Sema::hasVisibleMergedDefinition(NamedDecl *Def) {
Index: clang/include/clang/Basic/Module.h
===================================================================
--- clang/include/clang/Basic/Module.h
+++ clang/include/clang/Basic/Module.h
@@ -534,13 +534,13 @@
return Kind == ModuleInterfaceUnit || isModulePartition();
}
+ static StringRef getPrimaryModuleInterfaceName(StringRef Name) {
+ return Name.split(':').first;
+ }
+
/// Get the primary module interface name from a partition.
StringRef getPrimaryModuleInterfaceName() const {
- if (isModulePartition()) {
- auto pos = Name.find(':');
- return StringRef(Name.data(), pos);
- }
- return Name;
+ return getPrimaryModuleInterfaceName(Name);
}
/// Retrieve the full name of this module, including the path from
Index: clang/test/CXX/module/module.import/p2.cpp
===================================================================
--- /dev/null
+++ clang/test/CXX/module/module.import/p2.cpp
@@ -0,0 +1,21 @@
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: split-file %s %t
+// RUN: %clang_cc1 -std=c++20 %t/impl.cppm -emit-module-interface -o %t/M-impl.pcm
+// RUN: %clang_cc1 -std=c++20 %t/M.cppm -emit-module-interface -fprebuilt-module-path=%t -o %t/M.pcm
+// RUN: %clang_cc1 -std=c++20 %t/Use.cpp -fprebuilt-module-path=%t -verify -fsyntax-only
+
+//--- impl.cppm
+module M:impl;
+class A {};
+
+//--- M.cppm
+export module M;
+import :impl;
+export A f();
+
+//--- Use.cpp
+import M;
+void test() {
+ A a; // expected-error {{unknown type name 'A'}}
+}
Index: clang/lib/Sema/SemaLookup.cpp
===================================================================
--- clang/lib/Sema/SemaLookup.cpp
+++ clang/lib/Sema/SemaLookup.cpp
@@ -1561,12 +1561,11 @@
static bool isInCurrentModule(const Module *M, const LangOptions &LangOpts) {
// If M is the global module fragment of a module that we've not yet finished
// parsing, then it must be part of the current module.
- // If it's a partition, then it must be visible to an importer (since only
- // another partition or the named module can import it).
- return M->getTopLevelModuleName() == LangOpts.CurrentModule ||
- (M->Kind == Module::GlobalModuleFragment && !M->Parent) ||
- M->Kind == Module::ModulePartitionInterface ||
- M->Kind == Module::ModulePartitionImplementation;
+ if (M->isGlobalModule() && !M->Parent)
+ return true;
+
+ return M->getPrimaryModuleInterfaceName() ==
+ Module::getPrimaryModuleInterfaceName(LangOpts.CurrentModule);
}
bool Sema::hasVisibleMergedDefinition(NamedDecl *Def) {
Index: clang/include/clang/Basic/Module.h
===================================================================
--- clang/include/clang/Basic/Module.h
+++ clang/include/clang/Basic/Module.h
@@ -534,13 +534,13 @@
return Kind == ModuleInterfaceUnit || isModulePartition();
}
+ static StringRef getPrimaryModuleInterfaceName(StringRef Name) {
+ return Name.split(':').first;
+ }
+
/// Get the primary module interface name from a partition.
StringRef getPrimaryModuleInterfaceName() const {
- if (isModulePartition()) {
- auto pos = Name.find(':');
- return StringRef(Name.data(), pos);
- }
- return Name;
+ return getPrimaryModuleInterfaceName(Name);
}
/// Retrieve the full name of this module, including the path from
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
