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

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added inline comments.


================
Comment at: clang/lib/Lex/ModuleMap.cpp:910
+  return Result;
+}
+
----------------
The implementation looks similar to `createModuleForInterfaceUnit` really. It looks better to refactor it to `createModuleUnits` (or createModuleForCXX20Modules)  which containing an additional argument `Module::ModuleKind`. It would optimize the case for partitions too, which uses `createModuleForInterfaceUnit ` now and it is a little bit odd.


================
Comment at: clang/lib/Sema/SemaModule.cpp:306
+                                  Path[0].second);
+      // now create the implementation module
+      Mod = Map.createModuleForImplementationUnit(ModuleLoc, ModuleName,
----------------
I feel like the 3 comments are redundant.


================
Comment at: clang/lib/Sema/SemaModule.cpp:348-352
+  // We already potentially made an implicit import (in the case of a module
+  // implementation unit importing its interface).  Make this module visible
+  // and return the import decl to be added to the current TU.
+  if (Import)
+    VisibleModules.setVisible(Import->getImportedModule(), ModuleLoc);
----------------
We could move this logic to the place `Import` is created.


================
Comment at: clang/lib/Sema/SemaModule.cpp:355-357
+  // If we made an implicit import of the module interface, then return the
+  // imported module decl.
+  return Import ? ConvertDeclToDeclGroup(Import) : nullptr;
----------------
Is it necessary/helpful to return the import declaration? Although there is a FIXME, I think the current method looks a little bit confusing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126959