iains added inline comments.
================
Comment at: clang/include/clang/Basic/Module.h:537-543
+ static StringRef getPrimaryModuleInterfaceName(StringRef Name) {
+ return Name.split(':').first;
+ }
+
/// Get the primary module interface name from a partition.
StringRef getPrimaryModuleInterfaceName() const {
+ return getPrimaryModuleInterfaceName(getTopLevelModuleName());
----------------
I do not understand the purpose of this change, we already iterated over making
the function more efficient.
For C++20 modules there cannot be more than one parent - so that the nested
call to getTopLevelModule(), via getTopLevelModuleName() is redundant.
Is there some other case that needs special handling?
(If so then I think we should guard this at a higher level)
================
Comment at: clang/lib/Sema/SemaLookup.cpp:1567-1568
+
+ return M->getPrimaryModuleInterfaceName() ==
+ Module::getPrimaryModuleInterfaceName(LangOpts.CurrentModule);
}
----------------
... of course, "correctness before optimisation", but this test function seems
like it would benefit from some refactoring as suggested below.
it seems a bit unfortunate that we are placing a string comparison in the
"acceptable decl" lookup path, which might be made many times (the previous use
of the string compare was in making the module decl - which only happens once
in a TU).
* Sema has visibility of the import graph of modules (via DirectModuleImports
and Exports).
* We also know what the module parse state is (FirstDecl, GMF etc)
* If were are not parsing a module (i.e. LangOpts.CurrentModule.empty()) the we
could return false early?
* if ModuleScopes is empty we know we are not (yet) parsing a module
* Once we are parsing a module then we can test ModuleScopes.back() to find the
current module
there are only two users of isInCurrentModule() - so maybe we refactor could
make more use of the higher level state information - and / or cache
information on the parents of partitions to avoid the complexity of parsing
strings each time.
what do you think?
================
Comment at: clang/test/Modules/cxx20-10-1-ex2.cpp:62-69
+// 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.
----------------
@rsmith
so, I wonder if this is intended, it seems to defeat the mechanisms to avoid
circular dependencies.
TU1:
module M : Impl; // this is said not to generate a circular dependency because
M : Impl does not implicitly import M
import M; // but now we've done it manually ... and if that module is
implicitly exported to importers of the Impl. module interface....
....
TU2:
export module M;
import : Impl;
// ... then boom! we have a circular dep.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D123837/new/
https://reviews.llvm.org/D123837
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
