This revision was automatically updated to reflect the committed changes.
Closed by commit rL279096: Module: add -fprebuilt-module-path to support
loading prebuilt modules. (authored by mren).
Changed prior to commit:
https://reviews.llvm.org/D23125?vs=68456=68570#toc
Repository:
rL LLVM
manmanren added inline comments.
Comment at: lib/Frontend/CompilerInstance.cpp:1503
@@ +1502,3 @@
+if (!Module || !Module->getASTFile() ||
+std::string(Module->getASTFile()->getName()) != ModuleFileName) {
+ // Error out if Module does not refer to
rsmith added inline comments.
Comment at: lib/Frontend/CompilerInstance.cpp:1503
@@ +1502,3 @@
+if (!Module || !Module->getASTFile() ||
+std::string(Module->getASTFile()->getName()) != ModuleFileName) {
+ // Error out if Module does not refer to the
manmanren added a comment.
Cheers,
Manman
Comment at: lib/Frontend/CompilerInstance.cpp:1502
@@ +1501,3 @@
+Module = PP->getHeaderSearchInfo().lookupModule(ModuleName);
+if (!Module || !Module->getASTFile() ||
+
manmanren updated this revision to Diff 68456.
https://reviews.llvm.org/D23125
Files:
docs/Modules.rst
include/clang/Basic/DiagnosticCommonKinds.td
include/clang/Driver/Options.td
include/clang/Lex/HeaderSearch.h
include/clang/Lex/HeaderSearchOptions.h
rsmith added inline comments.
Comment at: lib/Frontend/CompilerInstance.cpp:1502
@@ +1501,3 @@
+Module = PP->getHeaderSearchInfo().lookupModule(ModuleName);
+if (!Module) {
+ getDiagnostics().Report(ModuleNameLoc, diag::err_module_prebuilt)
manmanren added a comment.
Thanks,
Manman
Comment at: lib/Frontend/CompilerInstance.cpp:1502
@@ +1501,3 @@
+Module = PP->getHeaderSearchInfo().lookupModule(ModuleName);
+if (!Module) {
+ getDiagnostics().Report(ModuleNameLoc, diag::err_module_prebuilt)
manmanren updated this revision to Diff 68442.
https://reviews.llvm.org/D23125
Files:
docs/Modules.rst
include/clang/Basic/DiagnosticCommonKinds.td
include/clang/Driver/Options.td
include/clang/Lex/HeaderSearch.h
include/clang/Lex/HeaderSearchOptions.h
rsmith added a comment.
Just a couple of things, but this basically LGTM.
Comment at: docs/Modules.rst:217
@@ +216,3 @@
+``-fprebuilt-module-path=``
+ Specify the path to the prebuilt modules. If specified, we will look for
modules in this directory for a given top-level
manmanren added a comment.
Richard,
Are you okay with the patch now?
Thanks,
Manman
https://reviews.llvm.org/D23125
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
manmanren added inline comments.
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 =
+
bruno added inline comments.
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 =
+
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
rsmith added inline comments.
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;
+
It would seem preferable to allow multiple of these,
manmanren updated this revision to Diff 67621.
manmanren added a comment.
Addressing Richard's comments.
https://reviews.llvm.org/D23125
Files:
docs/Modules.rst
include/clang/Driver/Options.td
include/clang/Lex/HeaderSearch.h
include/clang/Lex/HeaderSearchOptions.h
manmanren added a comment.
In https://reviews.llvm.org/D23125#510632, @rsmith wrote:
> This doesn't seem like quite the right user interface for this feature. The
> module cache is volatile, and it's not really reasonable for people to assume
> they know what is and isn't within it. Instead,
rsmith added inline comments.
Comment at: lib/Frontend/CompilerInstance.cpp:1483
@@ +1482,3 @@
+ HSOpts.ModulesUsePrebuiltModules ?
+ ASTReader::ARR_ConfigurationMismatch :
+
rsmith added a subscriber: rsmith.
rsmith added a comment.
This doesn't seem like quite the right user interface for this feature. The
module cache is volatile, and it's not really reasonable for people to assume
they know what is and isn't within it. Instead, it seems like what we want to
manmanren added a comment.
In https://reviews.llvm.org/D23125#504942, @benlangmuir wrote:
> How about -fmodules-use-prebuilt-**module-cache** for the flag name? Saying
> "prebuilt-modules" is confusing to me, since -fmodule-file can also be used
> to load a prebuilt module, but doesn't use
benlangmuir accepted this revision.
benlangmuir added a comment.
This revision is now accepted and ready to land.
Still missing the change for `if (IsPrebuilt) {`, but otherwise LGTM.
https://reviews.llvm.org/D23125
___
cfe-commits mailing list
manmanren updated this revision to Diff 67431.
manmanren added a comment.
Add comments for setting IsSystem to true.
https://reviews.llvm.org/D23125
Files:
include/clang/Basic/DiagnosticDriverKinds.td
include/clang/Driver/Options.td
include/clang/Lex/HeaderSearchOptions.h
benlangmuir added a comment.
Sorry for the delay, I apparently forgot to hit submit. Replied inline.
Comment at: lib/Frontend/CompilerInstance.cpp:1491
@@ +1490,3 @@
+false/*IsExplicit*/).first;
+Module->IsSystem = true;
+Module->IsFromModuleFile =
benlangmuir added a comment.
How about -fmodules-use-prebuilt-**module-cache** for the flag name? Saying
"prebuilt-modules" is confusing to me, since -fmodule-file can also be used to
load a prebuilt module, but doesn't use a cache.
Comment at: lib/Driver/Tools.cpp:5416
@@
manmanren added inline comments.
Comment at: lib/Driver/Tools.cpp:5416
@@ -5408,1 +5415,3 @@
+ if (Args.getLastArg(options::OPT_fmodules_use_prebuilt_modules)) {
+// When using prebuilt modules, we disable module hash.
benlangmuir wrote:
> ```
> if
manmanren created this revision.
manmanren added a reviewer: benlangmuir.
manmanren added a subscriber: cfe-commits.
We add -fmodules-use-prebuilt-modules to support using prebuilt modules. In
this mode, there is no need to load any module map and the programmer can
simply use "@import" syntax
25 matches
Mail list logo
