manmanren added a comment. Thanks, Manman
================ Comment at: include/clang/Lex/HeaderSearchOptions.h:96-97 @@ -95,1 +95,4 @@ + /// \brief The directory used to load prebuilt module files. + std::string PrebuiltModulePath; + ---------------- rsmith wrote: > It would seem preferable to allow multiple of these, to support mixing (for > instance) a path to prebuilt system modules and a path to some per-project > modules. I'd be fine handling that as a separate patch / discussion if you > prefer. It sounds reasonable to provide prebuilt system modules and prebuilt project modules. I assume via the same command-line option -fprebuilt-module-path. ================ Comment at: lib/Frontend/CompilerInstance.cpp:1438-1450 @@ -1437,3 +1437,15 @@ // Search for a module with the given name. Module = PP->getHeaderSearchInfo().lookupModule(ModuleName); - if (!Module) { + HeaderSearchOptions &HSOpts = + PP->getHeaderSearchInfo().getHeaderSearchOpts(); + + std::string ModuleFileName; + bool LoadFromPrebuiltModulePath = false; + if (Module) { + ModuleFileName = PP->getHeaderSearchInfo().getModuleFileName(Module); + } else if (!HSOpts.PrebuiltModulePath.empty()) { + // Try to load the module from the prebuilt module path. + ModuleFileName = + PP->getHeaderSearchInfo().getModuleFileName(ModuleName, "", true); + LoadFromPrebuiltModulePath = true; + } else { ---------------- rsmith wrote: > It looks like the logic here is: > > * if we found the module in a module map, then look for it in the module > cache > * otherwise, if we have a prebuilt module path, then look for it there > * otherwise, error > > I think that may be surprising; if I explicitly load a module map, perhaps > via `-fmodule-map-file=`, and supply a prebuilt form of that module in the > prebuilt modules path, I'd expect my prebuilt form to still be used. > > Perhaps instead we could first search the prebuilt module path for an > existing .pcm file, and if that fails and we have a `Module` then look for it > in the module cache? I was wondering about the priority of loading from prebuilt module path vs. loading from module cache. Your suggestion makes sense. ================ Comment at: lib/Frontend/CompilerInstance.cpp:1497-1505 @@ +1496,11 @@ + if (LoadFromPrebuiltModulePath && !Module) { + // Create a Module if we are using the prebuilt module path. + Module = PP->getHeaderSearchInfo().getModuleMap().findOrCreateModule( + ModuleName, nullptr, false/*IsFramework*/, + false/*IsExplicit*/).first; + // FIXME: Since we are mostly prebuilding system modules, we set + // IsSystem to true here. This is not always the correct choice, + // and IsSystem can affect diagnostics. + Module->IsSystem = true; + Module->IsFromModuleFile = true; + } ---------------- rsmith wrote: > The module file will have provided a `Module` instance, if it is in fact a > module file for the requested module. Instead of inventing a `Module` here, > can you check that one exists and produce an error if not? This is nice to know. I didn't realize we construct a Module when calling ReadAST. Can you be more specific on how to check if a Module exists? ================ Comment at: lib/Serialization/ASTReader.cpp:2273 @@ -2271,2 +2272,3 @@ if (DisableValidation || - (AllowConfigurationMismatch && Result == ConfigurationMismatch)) + ((AllowConfigurationMismatch || F.Kind == MK_PrebuiltModule) && + Result == ConfigurationMismatch)) ---------------- rsmith wrote: > You're allowing configuration mismatches that we don't consider to be > "compatible configuration mismatches" here. That seems really scary -- I > believe this allows using a C++ module from C and similar unreasonable > things, that will cause clang to crash. Did you really mean to do that? I originally had this in my testing patch to be really relaxing on prebuilt modules. I will remove this :] https://reviews.llvm.org/D23125 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits