ChuanqiXu updated this revision to Diff 423542.
ChuanqiXu added a comment.
Address comments:
- Add Cache in isInCurrentModule to avoid too many string comparisons.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D123837/new/
https://reviews.llvm.org/D123837
Files:
clang/include/clang/Basic/Module.h
clang/include/clang/Sema/Sema.h
clang/lib/Sema/SemaLookup.cpp
clang/test/CXX/module/module.import/p2.cpp
clang/test/Modules/cxx20-10-1-ex2.cpp
Index: clang/test/Modules/cxx20-10-1-ex2.cpp
===================================================================
--- clang/test/Modules/cxx20-10-1-ex2.cpp
+++ clang/test/Modules/cxx20-10-1-ex2.cpp
@@ -56,7 +56,14 @@
int &c = n; // expected-error {{use of undeclared identifier}}
//--- std10-1-ex2-tu7.cpp
+// expected-no-diagnostics
module B:X3; // does not implicitly import B
-import :X2; // X2 is an implementation so exports nothing.
- // error: n not visible here.
-int &c = n; // expected-error {{use of undeclared identifier }}
+import : X2; // X2 is an implementation unit import B.
+// According to [module.import]p7:
+// Additionally, when a module-import-declaration in a module unit of some
+// module M imports another module unit U of M, it also imports all
+// translation units imported by non-exported module-import-declarations in
+// the module unit purview of U.
+//
+// So B is imported in B:X3 due to B:X2 imported B. So n is visible here.
+int &c = n;
Index: clang/test/CXX/module/module.import/p2.cpp
===================================================================
--- /dev/null
+++ clang/test/CXX/module/module.import/p2.cpp
@@ -0,0 +1,38 @@
+// 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
+// RUN: %clang_cc1 -std=c++20 %t/UseInPartA.cppm -fprebuilt-module-path=%t -verify -fsyntax-only
+// RUN: %clang_cc1 -std=c++20 %t/UseInAnotherModule.cppm -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'}}
+}
+
+//--- UseInPartA.cppm
+// expected-no-diagnostics
+export module M:partA;
+import :impl;
+void test() {
+ A a;
+}
+
+//--- UseInAnotherModule.cppm
+export module B;
+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
@@ -1556,17 +1556,24 @@
return LookupModulesCache;
}
-/// Determine whether the module M is part of the current module from the
-/// perspective of a module-private visibility check.
-static bool isInCurrentModule(const Module *M, const LangOptions &LangOpts) {
+/// Determine whether the module M is in the same module with the current
+/// module.
+bool Sema::isInCurrentModule(const Module *M) {
+ assert(M && "We shouldn't check nullness for module here");
+ // Return quickly if we cached the result.
+ if (CurrentModuleUnitsCache.count(M))
+ return true;
+
// 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) ||
+ (M->getPrimaryModuleInterfaceName() ==
+ Module::getPrimaryModuleInterfaceName(getLangOpts().CurrentModule))) {
+ CurrentModuleUnitsCache.insert(M);
+ return true;
+ }
+
+ return false;
}
bool Sema::hasVisibleMergedDefinition(NamedDecl *Def) {
@@ -1578,7 +1585,7 @@
bool Sema::hasMergedDefinitionInCurrentModule(NamedDecl *Def) {
for (const Module *Merged : Context.getModulesWithMergedDefinition(Def))
- if (isInCurrentModule(Merged, getLangOpts()))
+ if (isInCurrentModule(Merged))
return true;
return false;
}
@@ -1760,7 +1767,7 @@
// means it is part of the current module. For any other query, that means it
// is in our visible module set.
if (ModulePrivate) {
- if (isInCurrentModule(M, getLangOpts()))
+ if (isInCurrentModule(M))
return true;
else if (M->Kind == Module::ModuleKind::ModulePartitionImplementation &&
isModuleDirectlyImported(M))
Index: clang/include/clang/Sema/Sema.h
===================================================================
--- clang/include/clang/Sema/Sema.h
+++ clang/include/clang/Sema/Sema.h
@@ -2274,6 +2274,12 @@
VisibleModuleSet VisibleModules;
+ /// Cache for module units which is in the same module with the current
+ /// module.
+ llvm::DenseSet<const Module *> CurrentModuleUnitsCache;
+
+ bool isInCurrentModule(const Module *M);
+
public:
/// Get the module owning an entity.
Module *getOwningModule(const Decl *Entity) {
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(getTopLevelModuleName());
}
/// Retrieve the full name of this module, including the path from
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
