ChuanqiXu added a comment.
I think we should add 2 more tests at least:
Import a partition a module purview but we failed to find one. And test
reflects the successful case.
================
Comment at: clang/include/clang/Sema/Sema.h:2989
/// \param ImportLoc The location of the 'import' keyword.
- /// \param Path The module access path.
+ /// \param NamePath The module toplevel name as an access path.
+ /// \param Partition The module partition name as an access path.
----------------
urnathan wrote:
> Is `NamePath` really a better name? You've not consistently changed all
> `Path`'s to this, and it doesn;t strike me as particularly mnemonic.
I use `Path` in my implementation.
================
Comment at: clang/lib/Sema/SemaDecl.cpp:1611
+ // If we have a decl in a module partition it is part of the containing
+ // module (which is the only thing that can be importing it).
----------------
We lack a comma here.
================
Comment at: clang/lib/Sema/SemaModule.cpp:224-225
+ if (IsPartition) {
+ // Create an interface, but note that it is an implementation
+ // unit.
Mod = Map.createModuleForInterfaceUnit(ModuleLoc, ModuleName,
----------------
I prefer to add more words here to tell why we create an interface unit but the
above condition is `ModuleDeclKind::Implementation`.
================
Comment at: clang/lib/Sema/SemaModule.cpp:260
+ ModuleScopes.back().ModuleInterface =
+ (MDK != ModuleDeclKind::Implementation || IsPartition);
+ ModuleScopes.back().IsPartition = IsPartition;
----------------
How about: `Mod->Kind != Module::ModulePartitionImplementation`.
================
Comment at: clang/lib/Sema/SemaModule.cpp:346-347
SourceLocation ImportLoc,
- ModuleIdPath Path) {
- // Flatten the module path for a C++20 or Modules TS module name.
+ ModuleIdPath NamePath,
+ ModuleIdPath Partition) {
+
----------------
I think we could change the signature of the lat 2 fields to
```
ModuleIdPath Path,
bool IsPartition)
```
I feel this is more simpler.
================
Comment at: clang/lib/Sema/SemaModule.cpp:350
+ bool IsPartition = !Partition.empty();
+ bool Cxx20Mode = getLangOpts().CPlusPlusModules || getLangOpts().ModulesTS;
+ assert((!IsPartition || Cxx20Mode) && "partition seen in non-C++20 code?");
----------------
I prefer to remove the support for getLangOpts().ModulesTS on the fly.
================
Comment at: clang/lib/Sema/SemaModule.cpp:359
+
std::string ModuleName;
+ if (IsPartition) {
----------------
I prefer to move the variable to the following block.
================
Comment at: clang/lib/Sema/SemaModule.cpp:361
+ if (IsPartition) {
+ // We already checked that we are in a module purview in the parser.
+ assert(!ModuleScopes.empty() && "in a module purview, but no module?");
----------------
I prefer to add an assertion:
```
assert (ModuleScopes.back().Module.isModulePurview());
```
================
Comment at: clang/lib/Sema/SemaModule.cpp:367-368
+ // owning named module.
+ size_t P = NamedMod->Name.find_first_of(":");
+ ModuleName = NamedMod->Name.substr(0, P + 1);
+ } else {
----------------
I would like to wrap this one as a method of class `Module`:
```
StringRef getPrimaryModuleName(Module *M) {
if (not partition)
return M->Name;
return M->Name.split(":").first;
}
```
================
Comment at: clang/lib/Sema/SemaModule.cpp:373
+ ModuleName = NamedMod->Name;
+ ModuleName += ":";
}
----------------
We could move this below the if statement.
================
Comment at: clang/lib/Sema/SemaModule.cpp:433-451
+ if (NamePath.empty()) {
+ // If this was a header import, pad out with dummy locations.
+ // FIXME: Pass in and use the location of the header-name token in this
+ // case.
+ for (Module *ModCheck = Mod; ModCheck; ModCheck = ModCheck->Parent)
IdentifierLocs.push_back(SourceLocation());
+ } else if (getLangOpts().CPlusPlusModules && !Mod->Parent) {
----------------
I don't find the reason to refactor here. It looks NFC and I think the original
form is simpler.
================
Comment at: clang/lib/Sema/SemaModule.cpp:477-480
+ } else if (getLangOpts().isCompilingModule()) {
+ Module *ThisModule = PP.getHeaderSearchInfo().lookupModule(
+ getLangOpts().CurrentModule, ExportLoc, false, false);
+ assert(ThisModule && "was expecting a module if building one");
----------------
What does it assert? I don't get the point.
================
Comment at: clang/test/Modules/cxx20-import-diagnostics-a.cpp:57
+module;
+
----------------
What if we don't add this? I think the original is good. So we should add a new
test if we feel needed.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D118586/new/
https://reviews.llvm.org/D118586
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
