[clang][lex] NFC: Extract module creation into function

[jansvoboda11]

Summary

This patch extracts the construction of Module within ModuleMap into a separate function.

[ahoppen]

I suppose the idea is that all Module creations should go through makeModule, right? In that case I think we should either

• make the Module constructor private and ModuleMap a friend of Module
• or at least add a doc comment to the Module constructor that says Modules should only be created using ModuleMap::makeModule.

Or are there other places that also create Modules but are not supposed to go through ModuleMap::makeModule?

[jansvoboda11]

In D116751#3227142, @ahoppen wrote:

I suppose the idea is that all Module creations should go through makeModule, right? In that case I think we should either

• make the Module constructor private and ModuleMap a friend of Module
• or at least add a doc comment to the Module constructor that says Modules should only be created using ModuleMap::makeModule.

Or are there other places that also create Modules but are not supposed to go through ModuleMap::makeModule?

The intent here is for all calls to the Module constructor in ModuleMap to go through makeModule(). This will make it possible to ensure a callback is always invoked when a Module is constructed in D113676.

Making the constructor private doesn't aid that goal, so I'd be inclined to do that in a separate patch. But would that be the right thing to do?
We don't instantiate Module outside of ModuleMap in the upstream repo, but I don't think there's anything fundamental that would prevent downstream projects doing that. Do we care about such use-cases though?

[ahoppen]

I just want to make sure that we don’t accidentally introduce a new instantiation of a Module that doesn’t go through makeModule in the future.

[jansvoboda11]

In D116751#3227243, @ahoppen wrote:

I just want to make sure that we don’t accidentally introduce a new instantiation of a Module that doesn’t go through makeModule in the future.

I understand that, but I'm not sure that's a desirable restriction in all situations. Moreover, Module resides in the clangBasic library while ModuleMap resides in clangLex. Befriending ModuleMap or otherwise tying Module to ModuleMap violates the library layering.

[ahoppen]

Ah, in that case it makes sense as-is. I just assumed they lived in the same library.

[jansvoboda11]

Abandoning this. We don't need to collect usage info from -fimplicit-module-maps because we don't use that feature during explicit builds: D120465. Support for usage collection for modules was removed entirely in D121295.

clang/include/clang/Lex/ModuleMap.h

private:
   llvm::DenseMap<const Module *, const FileEntry *> InferredModuleAllowedBy;
 
   llvm::DenseMap<const Module *, AdditionalModMapsSet> AdditionalModMaps;
 
   /// Describes whether we haved parsed a particular file as a module
   /// map.
   llvm::DenseMap<const FileEntry *, bool> ParsedModuleMap;
 
+  /// Creates new Module.
+  Module *makeModule(StringRef Name, SourceLocation DefinitionLoc,
+                     Module *Parent, bool IsFramework, bool IsExplicit,
+                     unsigned VisibilityID);
+
   /// Resolve the given export declaration into an actual export
   /// declaration.
   ///
   /// \param Mod The module in which we're resolving the export declaration.
   ///
   /// \param Unresolved The export declaration to resolve.
   ///
   /// \param Complain Whether this routine should complain about unresolvable

clang/lib/Lex/ModuleMap.cpp

ModuleMap::headerKindToRole(Module::HeaderKind Kind) {
   case Module::HK_PrivateTextual:
     return ModuleHeaderRole(PrivateHeader | TextualHeader);
   case Module::HK_Excluded:
     llvm_unreachable("unexpected header kind");
   }
   llvm_unreachable("unknown header kind");
 }
 
+Module *ModuleMap::makeModule(StringRef Name, SourceLocation DefinitionLoc,
+                              Module *Parent, bool IsFramework, bool IsExplicit,
+                              unsigned VisibilityID) {
+  return new Module(Name, DefinitionLoc, Parent, IsFramework, IsExplicit,
+                    VisibilityID);
+}
+
 Module::ExportDecl
 ModuleMap::resolveExport(Module *Mod,
                          const Module::UnresolvedExportDecl &Unresolved,
                          bool Complain) const {
   // We may have just a wildcard.
   if (Unresolved.Id.empty()) {
     assert(Unresolved.Wildcard && "Invalid unresolved export");
     return Module::ExportDecl(nullptr, true);
std::pair<Module *, bool> ModuleMap::findOrCreateModule(StringRef Name,
                                                         Module *Parent,
                                                         bool IsFramework,
                                                         bool IsExplicit) {
   // Try to find an existing module with this name.
   if (Module *Sub = lookupModuleQualified(Name, Parent))
     return std::make_pair(Sub, false);
 
   // Create a new module with this name.
-  Module *Result = new Module(Name, SourceLocation(), Parent, IsFramework,
+  Module *Result = makeModule(Name, SourceLocation(), Parent, IsFramework,
                               IsExplicit, NumCreatedModules++);
   if (!Parent) {
     if (LangOpts.CurrentModule == Name)
       SourceModule = Result;
     Modules[Name] = Result;
     ModuleScopeIDs[Result] = CurrentModuleScopeID;
   }
   return std::make_pair(Result, true);
 }
 
 Module *ModuleMap::createGlobalModuleFragmentForModuleUnit(SourceLocation Loc,
                                                            Module *Parent) {
-  auto *Result = new Module("<global>", Loc, Parent, /*IsFramework*/ false,
+  auto *Result = makeModule("<global>", Loc, Parent, /*IsFramework*/ false,
                             /*IsExplicit*/ true, NumCreatedModules++);
   Result->Kind = Module::GlobalModuleFragment;
   // If the created module isn't owned by a parent, send it to PendingSubmodules
   // to wait for its parent.
   if (!Result->Parent)
     PendingSubmodules.emplace_back(Result);
   return Result;
 }
 
 Module *
 ModuleMap::createPrivateModuleFragmentForInterfaceUnit(Module *Parent,
                                                        SourceLocation Loc) {
   auto *Result =
-      new Module("<private>", Loc, Parent, /*IsFramework*/ false,
+      makeModule("<private>", Loc, Parent, /*IsFramework*/ false,
                  /*IsExplicit*/ true, NumCreatedModules++);
   Result->Kind = Module::PrivateModuleFragment;
   return Result;
 }
 
 Module *ModuleMap::createModuleForInterfaceUnit(SourceLocation Loc,
                                                 StringRef Name,
                                                 Module *GlobalModule) {
   assert(LangOpts.CurrentModule == Name && "module name mismatch");
   assert(!Modules[Name] && "redefining existing module");
 
   auto *Result =
-      new Module(Name, Loc, nullptr, /*IsFramework*/ false,
+      makeModule(Name, Loc, nullptr, /*IsFramework*/ false,
                  /*IsExplicit*/ false, NumCreatedModules++);
   Result->Kind = Module::ModuleInterfaceUnit;
   Modules[Name] = SourceModule = Result;
 
   // Reparent the current global module fragment as a submodule of this module.
   for (auto &Submodule : PendingSubmodules) {
     Submodule->setParent(Result);
     Submodule.release(); // now owned by parent

 }
 
 Module *ModuleMap::createHeaderModule(StringRef Name,
                                       ArrayRef<Module::Header> Headers) {
   assert(LangOpts.CurrentModule == Name && "module name mismatch");
   assert(!Modules[Name] && "redefining existing module");
 
   auto *Result =
-      new Module(Name, SourceLocation(), nullptr, /*IsFramework*/ false,
+      makeModule(Name, SourceLocation(), nullptr, /*IsFramework*/ false,
                  /*IsExplicit*/ false, NumCreatedModules++);
   Result->Kind = Module::ModuleInterfaceUnit;
   Modules[Name] = SourceModule = Result;
 
   for (const Module::Header &H : Headers) {
-    auto *M = new Module(H.NameAsWritten, SourceLocation(), Result,
+    auto *M = makeModule(H.NameAsWritten, SourceLocation(), Result,
                          /*IsFramework*/ false,
                          /*IsExplicit*/ true, NumCreatedModules++);
     // Header modules are implicitly 'export *'.
     M->Exports.push_back(Module::ExportDecl(nullptr, true));
     addHeader(M, H, NormalHeader);
   }
 
   return Result;
Module *ModuleMap::inferFrameworkModule(const DirectoryEntry *FrameworkDir,
   auto UmbrellaHeader = FileMgr.getFile(UmbrellaName);
 
   // FIXME: If there's no umbrella header, we could probably scan the
   // framework to load *everything*. But, it's not clear that this is a good
   // idea.
   if (!UmbrellaHeader)
     return nullptr;
 
-  Module *Result = new Module(ModuleName, SourceLocation(), Parent,
+  Module *Result = makeModule(ModuleName, SourceLocation(), Parent,
                               /*IsFramework=*/true, /*IsExplicit=*/false,
                               NumCreatedModules++);
   InferredModuleAllowedBy[Result] = ModuleMapFile;
   Result->IsInferred = true;
   if (!Parent) {
     if (LangOpts.CurrentModule == ModuleName)
       SourceModule = Result;
     Modules[ModuleName] = Result;
Module *ModuleMap::inferFrameworkModule(const DirectoryEntry *FrameworkDir,
   return Result;
 }
 
 Module *ModuleMap::createShadowedModule(StringRef Name, bool IsFramework,
                                         Module *ShadowingModule) {
 
   // Create a new module with this name.
   Module *Result =
-      new Module(Name, SourceLocation(), /*Parent=*/nullptr, IsFramework,
+      makeModule(Name, SourceLocation(), /*Parent=*/nullptr, IsFramework,
                  /*IsExplicit=*/false, NumCreatedModules++);
   Result->ShadowingModule = ShadowingModule;
   Result->markUnavailable(/*Unimportable*/true);
   ModuleScopeIDs[Result] = CurrentModuleScopeID;
   ShadowModules.push_back(Result);
 
   return Result;
 }