[PATCH] D35020: [Modules] Add ability to specify module name to module file mapping

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.


================
Comment at: docs/ClangCommandLineReference.rst:3-4
   -------------------------------------------------------------------
   NOTE: This file is automatically generated by running clang-tblgen
   -gen-opt-docs. Do not edit this file by hand!!
   -------------------------------------------------------------------
----------------
Please revert the change to this file; we can regenerate it after this lands (as a separate commit).


================
Comment at: include/clang/Driver/Options.td:1105
+  Group<i_Group>, Flags<[DriverOption,CC1Option]>, MetaVarName<"[<name>=]<file>">,
+  HelpText<"Specify the mapping of module name to precompiled module file loading it if name is omitted.">;
 def fmodules_ignore_macro : Joined<["-"], "fmodules-ignore-macro=">, Group<f_Group>, Flags<[CC1Option]>,
----------------
How about "Specify the mapping of module name to precompiled module file, or load a module file if name is omitted."?


================
Comment at: include/clang/Serialization/ModuleManager.h:52-53
 
+  /// \brief All loaded prebuilt/explicit modules, indexed by module name.
+  std::map<std::string, ModuleFile *> PrebuiltModules;
+
----------------
`llvm::StringMap` would be preferable here.


================
Comment at: lib/Driver/ToolChains/Clang.cpp:3613-3622
+  // The -fmodule-file=<file> form can be used to load files containing
+  // precompiled modules.
+  for (const Arg *A : Args.filtered(options::OPT_fmodule_file)) {
+    StringRef Val = A->getValue();
+    if (Val.find('=') == StringRef::npos) {
       if (HaveAnyModules)
+        CmdArgs.push_back(Args.MakeArgString("-fmodule-file=" + Val));
----------------
What's the purpose of splitting this into two separate loops? I see that you're passing on all `-fmodule-file=name=value` flags even if modules is not enabled. Is that useful?


================
Comment at: lib/Frontend/CompilerInstance.cpp:1623-1630
+    /// FIXME: perhaps we should (a) look for a module using the module name
+    //  to file map (PrebuiltModuleFiles) and (b) diagnose if still not found?
+    //if (Module == nullptr) {
+    //  getDiagnostics().Report(ModuleNameLoc, diag::err_module_not_found)
+    //    << ModuleName;
+    //  ModuleBuildFailed = true;
+    //  return ModuleLoadResult();
----------------
This doesn't make sense for header modules (defined by a module map). For the case of a Modules TS module implementation unit, we should skip this `CurrentModule` check, but let's leave that for a separate patch.


================
Comment at: lib/Frontend/CompilerInvocation.cpp:982
+    StringRef Val = A->getValue();
+    if (Val.find('=') == StringRef::npos)
+      Opts.ExtraDeps.push_back(Val);
----------------
There should be some way to specify a module file that contains a `=` in its file name. Ideally, that way would be compatible with our currently-supported form of `-fmodule-name=filename`. I don't see a way to support that other than `stat`ing every value we're given and checking to see whether it exists before attempting to split on a `=`... thoughts?

One somewhat hackish alternative would be to only recognize an `=` if there is no preceding `/`. (So `-fmodule-file=foo=bar.pcm` specifies a mapping, and `-fmodule-file=./foo=bar.pcm` specifies the file `./foo=bar.pcm`.)

(FWIW, we also support module names containing `=` characters.)


================
Comment at: lib/Lex/HeaderSearch.cpp:131
 
 std::string HeaderSearch::getModuleFileName(Module *Module) {
   const FileEntry *ModuleMap =
----------------
I like the renaming you've done here. Should this one also be renamed to `getCachedModuleFileName`?


================
Comment at: lib/Serialization/ASTReader.cpp:4145-4146
+        // by-name lookup.
+        if (Type == MK_PrebuiltModule || Type == MK_ExplicitModule)
+          ModuleMgr.registerPrebuilt(F);
         break;
----------------
I'm worried by this: the `Type` can be different for different imports of the same .pcm file, and you'll only get here for the *first* import edge. So you can fail to add the module to the "prebuilt modules" map here, and then later attempt to look it up there (when processing the module offset map) and fail to find it.

Instead of keeping a separate lookup structure on the module manager, could you look up the `Module`, and then use its `ASTFile` to find the corresponding `ModuleFile`?

(If you go that way, the changes to module lookup when reading the module offset map could be generalized to apply to all `MK_*Module` types.)


https://reviews.llvm.org/D35020