[PATCH] D118586: [C++20][Modules][3/8] Initial handling for module partitions.

[Chuanqi Xu via Phabricator via cfe-commits]

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