[PATCH] D126959: [C++20][Modules] Introduce an implementation module.

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

I don't remember clearly if we have a test for the initializer in the implementation unit. Ignore this if we already have one.



================
Comment at: clang/include/clang/Basic/Module.h:137
     ImplicitGlobalModuleFragment,
   };
 
----------------
We may be able to save 1 bit for ModuleKind by adding two field bits to Module: `IsImplementation` and `IsPartition`. Then we can remove `ModulePartitionInterface` and `ModulePartitionImplementation`. Then let's rename `ModuleInterfaceUnit` to `NamedModuleUnit`.
So we can judge module interface unit, implementation unit,  module partition interface and module partition implementation by `NamedModuleUnit ` and the two bits.


================
Comment at: clang/lib/Sema/SemaDecl.cpp:1672-1677
+  // A module implementation unit has visibility of the decls in its implicitly
+  // imported interface.
+  if (getLangOpts().CPlusPlusModules && NewM && OldM &&
+      NewM->Kind == Module::ModuleImplementationUnit &&
+      OldM->Kind == Module::ModuleInterfaceUnit && NewM->Name == OldM->Name)
+    return false;
----------------
I feel slightly better to add a field in Sema:

```
Module *PrimaryModuleInterface = nullptr; // Only valid if we're parsing a module unit.
```

Then we can avoid the string compare here.  Also we can add it to `Sema::isUsableModule`. Now `Sema::isUsableModule` works because it falls to the string comparison.


================
Comment at: clang/lib/Sema/SemaModule.cpp:302
+  Module *Mod;                 // The module we are creating.
+  Module *Interface = nullptr; // The interface for an implementation.
   switch (MDK) {
----------------
Then we can avoid to declare this if we have `Sema::PrimaryModuleInterface `


================
Comment at: clang/lib/Sema/SemaModule.cpp:408-409
+    // module, if any.
+    if (!ModuleScopes.empty())
+      Context.addModuleInitializer(ModuleScopes.back().Module, Import);
+
----------------
ModuleScopes can't be empty here.


================
Comment at: clang/test/CXX/module/basic/basic.def.odr/p4.cppm:155-156
+
+  // FIXME: Issue #61427 Internal-linkage declarations in the interface TU
+  // should not be not visible here.
   (void)&static_var_module_linkage; // FIXME: Should not be visible here.
----------------
nit: I am a little bit tired to add FIXME in the tests... let's not try to add more...


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126959/new/

https://reviews.llvm.org/D126959