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

[Iain Sandoe via Phabricator via cfe-commits]

iains marked 10 inline comments as done.
iains added a comment.

General comments (at least my opinion).

1. One intention of this patch is to make Implementation Module Units regular (i.e. they should behave the same as any other module unit).  So I would tend to avoid changes that make this back into a "special case"

2. The lookup problems might be considered to be quite a serious bug, and so we should consider possible back porting and try to avoid making large changes in theses patches (once that problem is solved, then we can refactor, I think).

3. We do have tests for the initialiser in implementation units.



In D126959#4201152 <https://reviews.llvm.org/D126959#4201152>, @ChuanqiXu wrote:

> 



> And I guess it may refer to the problem we are discussing. The spec says the implementation unit will import the primary module interface implicitly.
> But the current implementation doesn't **import** it actually but loading it. This is inconsistent with the wording and it is not consistent with the design intention in clang.
> It resulted some problems we saw.

This patch implements the remaining pieces of that (including "as if by an `import`).



================
Comment at: clang/include/clang/Basic/Module.h:137
     ImplicitGlobalModuleFragment,
   };
 
----------------
ChuanqiXu wrote:
> 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.
Yes I agree we could do this, but let's keep refactoring as a follow-on job.


================
Comment at: clang/lib/Frontend/FrontendActions.cpp:835
+  case Module::ModuleImplementationUnit:
+    return "Implementation Unit (should never be serialized)";
   case Module::ModulePartitionInterface:
----------------
ChuanqiXu wrote:
> Since `ModuleKindName()` here is used as a helpful for printers, it looks odd to contain the `should never be serialized` part. I think we should check this when we try to generate PCM files for ModuleImplementationUnit.
OK. I can remove that

We do already check and refuse to generate a PCM for a module implementation (if you put `-emit-module-interface`, and the file contains `module Foo;` it will be rejected).



================
Comment at: clang/lib/Lex/ModuleMap.cpp:910
+  return Result;
+}
+
----------------
ChuanqiXu wrote:
> 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.
OK I did think about refactoring that, but decided to keep the changes simple for now.  I am happy to look at refactoring.


================
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;
----------------
ChuanqiXu wrote:
> 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.
I also thought about doing this, but decided that we should try to keep the interface regular - so that all modules are treated the same.

If we find that the string comparison for primary module name is a "hot spot" then we should address it a different way - by caching an identifier or similar (since we need to compare it in other places too).


================
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) {
----------------
ChuanqiXu wrote:
> Then we can avoid to declare this if we have `Sema::PrimaryModuleInterface `
see comment above.


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


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


================
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);
----------------
ChuanqiXu wrote:
> We could move this logic to the place `Import` is created.
We cannot move all of it - since some of the information is being added to the module that we create after the import.


================
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;
----------------
ChuanqiXu wrote:
> Is it necessary/helpful to return the import declaration? Although there is a FIXME, I think the current method looks a little bit confusing.
The FIXME predates our work on this, at the moment I do not know what the idea was for that - I am happy to remove it now - the current implementation does not need a module decl.



================
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.
----------------
ChuanqiXu wrote:
> nit: I am a little bit tired to add FIXME in the tests... let's not try to add more...
I agree that FIXMEs in tests are likely to be forgotten, so that they are not so much use.

However, we have a situation in which the test needs to be incorrect in order to pass at the present time; we do not have a good mechanism for noting this - since if we add the expected-error, then the test will fail - so we'd have to XFAIL it - which would mean that we would lose test coverage for the cases that do work.

If you prefer, I can remove the FIXME.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126959