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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
