[PATCH] D37299: [Modules] Add ability to specify module name to module file mapping in a file
boris added a comment. Ping. Also, to add to my previous comment, even if for a moment we ignore the header dependencies and when they are extracted, a modern build system normally tracks other kinds of what can be called "auxiliary dependency information": some form of compiler signature, hash of options that were used to compile the file, etc., so that when any of those change, the object file gets recompiled automatically. For example, in build2, we store all these signatures/hashes plus the header and module dependency information in a single .d file (which we call auxiliary dependency database). What I am trying to show by this is that it is well established that for C/C++ compilation there has to be an extra file for each .o where such information is stored. And it seems natural to want to reuse this file for supplying the "module map" to the compiler instead of creating yet another per-.o file. https://reviews.llvm.org/D37299 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37299: [Modules] Add ability to specify module name to module file mapping in a file
boris added a comment. Hi Richard, Thanks for still entertaining the idea. Yes, I believe, in order to support modules (TS) the build system will have to extract module (and header while at it) dependency information prior to compilation rather than as a byproduct of compilation (which is how most build systems do it now). I've written on how all this can work (as well as the support that would be required from the compiler) here: https://build2.org/article/cxx-modules-misconceptions.xhtml#build FWIW, we do it this way in build2 (and already have a .d file that we would like to reuse) and get 3x speedup using Clang on a modularized build. Also Gaby told me that he is working on a tool for VC that will do extraction of header and module dependency information (as a single preprocessor pass). https://reviews.llvm.org/D37299 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37299: [Modules] Add ability to specify module name to module file mapping in a file
rsmith added a comment. I'm still unconvinced that this mechanism is worth supporting. The use case of putting this information into .d files doesn't make sense to me. .d files are generally outputs of prior compilations, whereas this information is a compilation input, needed in every compilation (including the first), and must be eagerly updated if a change to the source code adds a new dependency. https://reviews.llvm.org/D37299 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37299: [Modules] Add ability to specify module name to module file mapping in a file
boris added a comment. Ping. https://reviews.llvm.org/D37299 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37299: [Modules] Add ability to specify module name to module file mapping in a file
boris added a comment. Ping. https://reviews.llvm.org/D37299 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37299: [Modules] Add ability to specify module name to module file mapping in a file
boris added a comment. Ping. https://reviews.llvm.org/D37299 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37299: [Modules] Add ability to specify module name to module file mapping in a file
boris added a comment. Ping. https://reviews.llvm.org/D37299 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37299: [Modules] Add ability to specify module name to module file mapping in a file
boris added a comment. Ping. https://reviews.llvm.org/D37299 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37299: [Modules] Add ability to specify module name to module file mapping in a file
boris added a comment. Ping. https://reviews.llvm.org/D37299 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37299: [Modules] Add ability to specify module name to module file mapping in a file
boris updated this revision to Diff 116442.
boris added a comment.
Another attempt to upload a clean diff (also rebased on latest HEAD).
https://reviews.llvm.org/D37299
Files:
docs/Modules.rst
include/clang/Basic/DiagnosticDriverKinds.td
include/clang/Driver/Options.td
lib/Driver/ToolChains/Clang.cpp
lib/Frontend/CompilerInvocation.cpp
test/CXX/modules-ts/basic/basic.search/module-import.cpp
Index: test/CXX/modules-ts/basic/basic.search/module-import.cpp
===
--- test/CXX/modules-ts/basic/basic.search/module-import.cpp
+++ test/CXX/modules-ts/basic/basic.search/module-import.cpp
@@ -5,6 +5,8 @@
// RUN: echo 'export module x; int a, b;' > %t/x.cppm
// RUN: echo 'export module y; import x; int c;' > %t/y.cppm
// RUN: echo 'export module z; import y; int d;' > %t/z.cppm
+// RUN: echo 'x=%t/x.pcm' > %t/modmap
+// RUN: echo 'y=%t/y.pcm' >> %t/modmap
//
// RUN: %clang_cc1 -std=c++1z -fmodules-ts -emit-module-interface %t/x.cppm -o %t/x.pcm
// RUN: %clang_cc1 -std=c++1z -fmodules-ts -emit-module-interface -fmodule-file=%t/x.pcm %t/y.cppm -o %t/y.pcm
@@ -19,7 +21,17 @@
// RUN: %clang_cc1 -std=c++1z -fmodules-ts -I%t -fmodule-file=y=%t/y.pcm -verify %s \
// RUN:-DMODULE_NAME=y
//
+// RUN: %clang_cc1 -std=c++1z -fmodules-ts -I%t -fmodule-file-map=%t/modmap -verify %s \
+// RUN:-DMODULE_NAME=x
+// RUN: %clang_cc1 -std=c++1z -fmodules-ts -I%t -fmodule-file-map=%t/modmap -verify %s \
+// RUN:-DMODULE_NAME=y
+//
// RUN: mv %t/x.pcm %t/a.pcm
+// RUN: echo 'foo.o: foo.cxx' > %t/modmap
+// RUN: echo '# Mmodule name to file mapping:' >> %t/modmap
+// RUN: echo '#@z=%t/z.pcm'>> %t/modmap
+// RUN: echo '#@ y=%t/y.pcm' >> %t/modmap
+// RUN: echo '#@x=%t/a.pcm ' >> %t/modmap
//
// RUN: %clang_cc1 -std=c++1z -fmodules-ts -I%t -fmodule-file=x=%t/a.pcm -verify %s \
// RUN:-DMODULE_NAME=x
@@ -33,6 +45,14 @@
// RUN: %clang_cc1 -std=c++1z -fmodules-ts -I%t -fmodule-file=z=%t/z.pcm -fmodule-file=y=%t/y.pcm -fmodule-file=x=%t/a.pcm -verify %s \
// RUN:-DMODULE_NAME=z
//
+//
+// RUN: %clang_cc1 -std=c++1z -fmodules-ts -I%t -fmodule-file-map=#@=%t/modmap -verify %s \
+// RUN:-DMODULE_NAME=x
+// RUN: %clang_cc1 -std=c++1z -fmodules-ts -I%t -fmodule-file-map=#@=%t/modmap -verify %s \
+// RUN:-DMODULE_NAME=y
+// RUN: %clang_cc1 -std=c++1z -fmodules-ts -I%t -fmodule-file-map=#@=%t/modmap -verify %s \
+// RUN:-DMODULE_NAME=z
+//
import MODULE_NAME;
Index: lib/Frontend/CompilerInvocation.cpp
===
--- lib/Frontend/CompilerInvocation.cpp
+++ lib/Frontend/CompilerInvocation.cpp
@@ -1531,8 +1531,80 @@
return P.str();
}
+// Read the mapping of module names to precompiled module files from a file.
+// The argument can include an optional line prefix ([]=), in
+// which case only lines that start with the prefix are considered (with the
+// prefix and the following whitespaces, if any, ignored).
+//
+// Each mapping entry should be in the same form as the -fmodule-file option
+// value (=) with leading/trailing whitespaces ignored.
+//
+static void LoadModuleFileMap(HeaderSearchOptions ,
+ DiagnosticsEngine , FileManager ,
+ StringRef Val, const std::string ) {
+ // See if we have the prefix.
+ StringRef File;
+ StringRef Prefix;
+ if (Val.find('=') != StringRef::npos) {
+auto Pair = Val.split('=');
+Prefix = Pair.first;
+File = Pair.second;
+if (Prefix.empty()) {
+ Diags.Report(diag::err_drv_invalid_value) << Arg << Val;
+ return;
+}
+ } else
+File = Val;
+
+ if (File.empty()) {
+Diags.Report(diag::err_drv_invalid_value) << Arg << Val;
+return;
+ }
+
+ auto *Buf = FileMgr.getBufferForFile(File);
+ if (!Buf) {
+Diags.Report(diag::err_cannot_open_file)
+<< File << Buf.getError().message();
+return;
+ }
+
+ // Read the file line by line.
+ StringRef Str = Buf.get()->getBuffer();
+ for (size_t B = 0, E = 0; B < Str.size(); B = E + 1) {
+E = Str.find_first_of(StringRef("\n\0", 2), B);
+
+if (E == StringRef::npos)
+ E = Str.size();
+else if (Str[E] == '\0')
+ break; // The file (or the rest of it) is binary, bail out.
+
+// [B, E) is our line. Compare and skip the prefix, if any.
+StringRef Line = Str.substr(B, E - B);
+if (!Prefix.empty()) {
+ if (!Line.startswith(Prefix))
+continue;
+
+ Line = Line.substr(Prefix.size());
+}
+
+// Skip leading and trailing whitespaces and ignore blanks (even if they
+// had prefix; think make comments).
+Line = Line.trim();
+if (Line.empty())
+ continue;
+
+if (Line.find('=') == StringRef::npos) {
+ Diags.Report(diag::err_drv_invalid_module_file_map) << Line;
+
[PATCH] D37299: [Modules] Add ability to specify module name to module file mapping in a file
boris updated this revision to Diff 116438.
boris marked an inline comment as done.
boris added a comment.
New revision this time with the tests (which got dropped from the earlier
revision diff for some reason).
https://reviews.llvm.org/D37299
Files:
docs/Modules.rst
include/clang/Basic/DiagnosticDriverKinds.td
include/clang/Driver/Options.td
lib/Driver/ToolChains/Clang.cpp
lib/Frontend/CompilerInvocation.cpp
test/CXX/modules-ts/basic/basic.search/module-import.cpp
Index: test/CXX/modules-ts/basic/basic.search/module-import.cpp
===
--- test/CXX/modules-ts/basic/basic.search/module-import.cpp
+++ test/CXX/modules-ts/basic/basic.search/module-import.cpp
@@ -5,6 +5,8 @@
// RUN: echo 'export module x; int a, b;' > %t/x.cppm
// RUN: echo 'export module y; import x; int c;' > %t/y.cppm
// RUN: echo 'export module z; import y; int d;' > %t/z.cppm
+// RUN: echo 'x=%t/x.pcm' > %t/modmap
+// RUN: echo 'y=%t/y.pcm' >> %t/modmap
//
// RUN: %clang_cc1 -std=c++1z -fmodules-ts -emit-module-interface %t/x.cppm -o %t/x.pcm
// RUN: %clang_cc1 -std=c++1z -fmodules-ts -emit-module-interface -fmodule-file=%t/x.pcm %t/y.cppm -o %t/y.pcm
@@ -19,7 +21,17 @@
// RUN: %clang_cc1 -std=c++1z -fmodules-ts -I%t -fmodule-file=y=%t/y.pcm -verify %s \
// RUN:-DMODULE_NAME=y
//
+// RUN: %clang_cc1 -std=c++1z -fmodules-ts -I%t -fmodule-file-map=%t/modmap -verify %s \
+// RUN:-DMODULE_NAME=x
+// RUN: %clang_cc1 -std=c++1z -fmodules-ts -I%t -fmodule-file-map=%t/modmap -verify %s \
+// RUN:-DMODULE_NAME=y
+//
// RUN: mv %t/x.pcm %t/a.pcm
+// RUN: echo 'foo.o: foo.cxx' > %t/modmap
+// RUN: echo '# Mmodule name to file mapping:' >> %t/modmap
+// RUN: echo '#@z=%t/z.pcm'>> %t/modmap
+// RUN: echo '#@ y=%t/y.pcm' >> %t/modmap
+// RUN: echo '#@x=%t/a.pcm ' >> %t/modmap
//
// RUN: %clang_cc1 -std=c++1z -fmodules-ts -I%t -fmodule-file=x=%t/a.pcm -verify %s \
// RUN:-DMODULE_NAME=x
@@ -33,7 +45,15 @@
// RUN: %clang_cc1 -std=c++1z -fmodules-ts -I%t -fmodule-file=z=%t/z.pcm -fmodule-file=y=%t/y.pcm -fmodule-file=x=%t/a.pcm -verify %s \
// RUN:-DMODULE_NAME=z
//
+//
+// RUN: %clang_cc1 -std=c++1z -fmodules-ts -I%t -fmodule-file-map=#@=%t/modmap -verify %s \
+// RUN:-DMODULE_NAME=x
+// RUN: %clang_cc1 -std=c++1z -fmodules-ts -I%t -fmodule-file-map=#@=%t/modmap -verify %s \
+// RUN:-DMODULE_NAME=y
+// RUN: %clang_cc1 -std=c++1z -fmodules-ts -I%t -fmodule-file-map=#@=%t/modmap -verify %s \
+// RUN:-DMODULE_NAME=z
+//
import MODULE_NAME;
// expected-no-diagnostics
Index: lib/Frontend/CompilerInvocation.cpp
===
--- lib/Frontend/CompilerInvocation.cpp
+++ lib/Frontend/CompilerInvocation.cpp
@@ -1528,8 +1528,80 @@
return P.str();
}
+// Read the mapping of module names to precompiled module files from a file.
+// The argument can include an optional line prefix ([]=), in
+// which case only lines that start with the prefix are considered (with the
+// prefix and the following whitespaces, if any, ignored).
+//
+// Each mapping entry should be in the same form as the -fmodule-file option
+// value (=) with leading/trailing whitespaces ignored.
+//
+static void LoadModuleFileMap(HeaderSearchOptions ,
+ DiagnosticsEngine , FileManager ,
+ StringRef Val, const std::string ) {
+ // See if we have the prefix.
+ StringRef File;
+ StringRef Prefix;
+ if (Val.find('=') != StringRef::npos) {
+auto Pair = Val.split('=');
+Prefix = Pair.first;
+File = Pair.second;
+if (Prefix.empty()) {
+ Diags.Report(diag::err_drv_invalid_value) << Arg << Val;
+ return;
+}
+ } else
+File = Val;
+
+ if (File.empty()) {
+Diags.Report(diag::err_drv_invalid_value) << Arg << Val;
+return;
+ }
+
+ auto *Buf = FileMgr.getBufferForFile(File);
+ if (!Buf) {
+Diags.Report(diag::err_cannot_open_file)
+<< File << Buf.getError().message();
+return;
+ }
+
+ // Read the file line by line.
+ StringRef Str = Buf.get()->getBuffer();
+ for (size_t B = 0, E = 0; B < Str.size(); B = E + 1) {
+E = Str.find_first_of(StringRef("\n\0", 2), B);
+
+if (E == StringRef::npos)
+ E = Str.size();
+else if (Str[E] == '\0')
+ break; // The file (or the rest of it) is binary, bail out.
+
+// [B, E) is our line. Compare and skip the prefix, if any.
+StringRef Line = Str.substr(B, E - B);
+if (!Prefix.empty()) {
+ if (!Line.startswith(Prefix))
+continue;
+
+ Line = Line.substr(Prefix.size());
+}
+
+// Skip leading and trailing whitespaces and ignore blanks (even if they
+// had prefix; think make comments).
+Line = Line.trim();
+if (Line.empty())
+ continue;
+
+if (Line.find('=') ==
[PATCH] D37299: [Modules] Add ability to specify module name to module file mapping in a file
boris added a comment. Yes, the main "feature" of this approach compared to @file is the ability to reuse an already existing file to store this information. Most build systems that support C/C++ compilation have to store auxiliary dependency information at least for the extracted header dependencies (those .d files generated by the -M option family, for example) but some also store hashes of options, compiler version/signature, etc. So instead of creating a yet another file (per translation unit), the idea is to reuse the already existing one by storing the mapping with some "distinguishing" prefix. As a concrete example, a make-based build system could append it to the .d file (which is a makefile fragment) as comments. How exactly this information is extracted is still an open question but I think this approach is generic enough to accommodate a wide range of possibilities (for example, -M could produce this information or the build system could append it itself after the -M is done). https://reviews.llvm.org/D37299 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37299: [Modules] Add ability to specify module name to module file mapping in a file
rsmith added a comment. I wonder whether this feature really pulls its weight compared to using `@file` and prepending `-fmodule-file=` to each line. The big difference between this patch and an `@file` seems to be the filtering by prefix / storing this information as a subset of the data in a file. Can you say a bit more about how you're envisioning this integrating into build systems, such that that would be a natural way to model this mapping? https://reviews.llvm.org/D37299 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37299: [Modules] Add ability to specify module name to module file mapping in a file
bruno added a comment.
Hi Boris,
This is a handy option, very nice. Can you also add testcases?
Comment at: lib/Frontend/CompilerInvocation.cpp:1561
+
+ auto Buf = FileMgr.getBufferForFile(File);
+ if (!Buf) {
`auto Buf` -> `auto *Buf`
https://reviews.llvm.org/D37299
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37299: [Modules] Add ability to specify module name to module file mapping in a file
boris added a comment. Ping. https://reviews.llvm.org/D37299 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37299: [Modules] Add ability to specify module name to module file mapping in a file
boris added a comment. Ping. https://reviews.llvm.org/D37299 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37299: [Modules] Add ability to specify module name to module file mapping in a file
boris updated this revision to Diff 113779.
boris marked an inline comment as done.
boris added a comment.
New patch revision with David's comments addressed.
https://reviews.llvm.org/D37299
Files:
docs/Modules.rst
include/clang/Basic/DiagnosticDriverKinds.td
include/clang/Driver/Options.td
lib/Driver/ToolChains/Clang.cpp
lib/Frontend/CompilerInvocation.cpp
Index: lib/Frontend/CompilerInvocation.cpp
===
--- lib/Frontend/CompilerInvocation.cpp
+++ lib/Frontend/CompilerInvocation.cpp
@@ -1528,8 +1528,80 @@
return P.str();
}
+// Read the mapping of module names to precompiled module files from a file.
+// The argument can include an optional line prefix ([]=), in
+// which case only lines that start with the prefix are considered (with the
+// prefix and the following whitespaces, if any, ignored).
+//
+// Each mapping entry should be in the same form as the -fmodule-file option
+// value (=) with leading/trailing whitespaces ignored.
+//
+static void LoadModuleFileMap(HeaderSearchOptions ,
+ DiagnosticsEngine , FileManager ,
+ StringRef Val, const std::string ) {
+ // See if we have the prefix.
+ StringRef File;
+ StringRef Prefix;
+ if (Val.find('=') != StringRef::npos) {
+auto Pair = Val.split('=');
+Prefix = Pair.first;
+File = Pair.second;
+if (Prefix.empty()) {
+ Diags.Report(diag::err_drv_invalid_value) << Arg << Val;
+ return;
+}
+ } else
+File = Val;
+
+ if (File.empty()) {
+Diags.Report(diag::err_drv_invalid_value) << Arg << Val;
+return;
+ }
+
+ auto Buf = FileMgr.getBufferForFile(File);
+ if (!Buf) {
+Diags.Report(diag::err_cannot_open_file)
+<< File << Buf.getError().message();
+return;
+ }
+
+ // Read the file line by line.
+ StringRef Str = Buf.get()->getBuffer();
+ for (size_t B = 0, E = 0; B < Str.size(); B = E + 1) {
+E = Str.find_first_of(StringRef("\n\0", 2), B);
+
+if (E == StringRef::npos)
+ E = Str.size();
+else if (Str[E] == '\0')
+ break; // The file (or the rest of it) is binary, bail out.
+
+// [B, E) is our line. Compare and skip the prefix, if any.
+StringRef Line = Str.substr(B, E - B);
+if (!Prefix.empty()) {
+ if (!Line.startswith(Prefix))
+continue;
+
+ Line = Line.substr(Prefix.size());
+}
+
+// Skip leading and trailing whitespaces and ignore blanks (even if they
+// had prefix; think make comments).
+Line = Line.trim();
+if (Line.empty())
+ continue;
+
+if (Line.find('=') == StringRef::npos) {
+ Diags.Report(diag::err_drv_invalid_module_file_map) << Line;
+ continue;
+}
+
+Opts.PrebuiltModuleFiles.insert(Line.split('='));
+ }
+}
+
static void ParseHeaderSearchArgs(HeaderSearchOptions , ArgList ,
- const std::string ) {
+ DiagnosticsEngine ,
+ FileManager ) {
using namespace options;
Opts.Sysroot = Args.getLastArgValue(OPT_isysroot, "/");
Opts.Verbose = Args.hasArg(OPT_v);
@@ -1543,6 +1615,7 @@
// Canonicalize -fmodules-cache-path before storing it.
SmallString<128> P(Args.getLastArgValue(OPT_fmodules_cache_path));
if (!(P.empty() || llvm::sys::path::is_absolute(P))) {
+const std::string (FileMgr.getFileSystemOpts().WorkingDir);
if (WorkingDir.empty())
llvm::sys::fs::make_absolute(P);
else
@@ -1558,6 +1631,8 @@
if (Val.find('=') != StringRef::npos)
Opts.PrebuiltModuleFiles.insert(Val.split('='));
}
+ for (const Arg *A : Args.filtered(OPT_fmodule_file_map))
+LoadModuleFileMap(Opts, Diags, FileMgr, A->getValue(), A->getAsString(Args));
for (const Arg *A : Args.filtered(OPT_fprebuilt_module_path))
Opts.AddPrebuiltModulePath(A->getValue());
Opts.DisableModuleHash = Args.hasArg(OPT_fdisable_module_hash);
@@ -2511,7 +2586,6 @@
}
static void ParsePreprocessorArgs(PreprocessorOptions , ArgList ,
- FileManager ,
DiagnosticsEngine ,
frontend::ActionKind Action) {
using namespace options;
@@ -2680,14 +2754,19 @@
false /*DefaultDiagColor*/, false /*DefaultShowOpt*/);
ParseCommentArgs(LangOpts.CommentOpts, Args);
ParseFileSystemArgs(Res.getFileSystemOpts(), Args);
+
+ // File manager used during option parsing (e.g., for loading map files,
+ // etc).
+ //
+ FileManager FileMgr(Res.getFileSystemOpts());
+
// FIXME: We shouldn't have to pass the DashX option around here
InputKind DashX = ParseFrontendArgs(Res.getFrontendOpts(), Args, Diags,
LangOpts.IsHeaderFile);
ParseTargetArgs(Res.getTargetOpts(), Args, Diags);
Success &= ParseCodeGenArgs(Res.getCodeGenOpts(), Args, DashX, Diags,
[PATCH] D37299: [Modules] Add ability to specify module name to module file mapping in a file
boris marked 3 inline comments as done.
boris added a comment.
David, thanks for the review. Uploading the new revision (also rebased on HEAD).
Comment at: lib/Frontend/CompilerInvocation.cpp:1576
+ StringRef Str = Buf.get()->getBuffer();
+ for (size_t B(0), E(B); B < Str.size(); B = E + 1)
+ {
dblaikie wrote:
> Prefer copy init over direct init:
>
> for (size_t B = 0, E = 0;
>
> & probably != rather than < is more canonical/common in the LLVM codebase.
All done except the != change -- B can actually go one over size (see the npos
case in the loop body).
https://reviews.llvm.org/D37299
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37299: [Modules] Add ability to specify module name to module file mapping in a file
dblaikie added a comment.
Couple of drive by comments - no doubt Richard will have a more in depth
review, but figured this might save a round or two.
Comment at: lib/Frontend/CompilerInvocation.cpp:1561-1565
+ if (File.empty())
+ {
+Diags.Report(diag::err_drv_invalid_value) << Arg << Val;
+return;
+ }
Run the whole change through clang-format (there are tools to help do this over
a patch or git revision range) so it matches LLVM coding conventions
Comment at: lib/Frontend/CompilerInvocation.cpp:1576
+ StringRef Str = Buf.get()->getBuffer();
+ for (size_t B(0), E(B); B < Str.size(); B = E + 1)
+ {
Prefer copy init over direct init:
for (size_t B = 0, E = 0;
& probably != rather than < is more canonical/common in the LLVM codebase.
Comment at: lib/Frontend/CompilerInvocation.cpp:1586
+// [B, E) is our line. Compare and skip the prefix, if any.
+StringRef Line(Str.data() + B, E - B);
+if (!Prefix.empty())
There's probably a StringRef-y way to do this rather than going back through
raw pointers? (StringRef::substr or the like)
https://reviews.llvm.org/D37299
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37299: [Modules] Add ability to specify module name to module file mapping in a file
boris created this revision.
Add the -fmodule-file-map=[=] option which can be used to specify
a file that contains module name to precompiled modules files mapping, similar
to -fmodule-file==. The can be used to only consider
certain lines which can be useful if we want to store the mapping in an already
existing file (for example, as comments in the .d makefile fragment generated
with the -M option or some such).
-
Additional notes:
1. Based on this mailing list discussion:
http://lists.llvm.org/pipermail/cfe-dev/2017-June/054431.html
2. Based on the functionality implemented in: https://reviews.llvm.org/D35020
https://reviews.llvm.org/D37299
Files:
docs/Modules.rst
include/clang/Basic/DiagnosticDriverKinds.td
include/clang/Driver/Options.td
lib/Driver/ToolChains/Clang.cpp
lib/Frontend/CompilerInvocation.cpp
test/CXX/modules-ts/basic/basic.search/module-import.cpp
Index: test/CXX/modules-ts/basic/basic.search/module-import.cpp
===
--- test/CXX/modules-ts/basic/basic.search/module-import.cpp
+++ test/CXX/modules-ts/basic/basic.search/module-import.cpp
@@ -5,6 +5,8 @@
// RUN: echo 'export module x; int a, b;' > %t/x.cppm
// RUN: echo 'export module y; import x; int c;' > %t/y.cppm
// RUN: echo 'export module z; import y; int d;' > %t/z.cppm
+// RUN: echo 'x=%t/x.pcm' > %t/modmap
+// RUN: echo 'y=%t/y.pcm' >> %t/modmap
//
// RUN: %clang_cc1 -std=c++1z -fmodules-ts -emit-module-interface %t/x.cppm -o %t/x.pcm
// RUN: %clang_cc1 -std=c++1z -fmodules-ts -emit-module-interface -fmodule-file=%t/x.pcm %t/y.cppm -o %t/y.pcm
@@ -19,7 +21,17 @@
// RUN: %clang_cc1 -std=c++1z -fmodules-ts -I%t -fmodule-file=y=%t/y.pcm -verify %s \
// RUN:-DMODULE_NAME=y
//
+// RUN: %clang_cc1 -std=c++1z -fmodules-ts -I%t -fmodule-file-map=%t/modmap -verify %s \
+// RUN:-DMODULE_NAME=x
+// RUN: %clang_cc1 -std=c++1z -fmodules-ts -I%t -fmodule-file-map=%t/modmap -verify %s \
+// RUN:-DMODULE_NAME=y
+//
// RUN: mv %t/x.pcm %t/a.pcm
+// RUN: echo 'foo.o: foo.cxx' > %t/modmap
+// RUN: echo '# Mmodule name to file mapping:' >> %t/modmap
+// RUN: echo '#@z=%t/z.pcm'>> %t/modmap
+// RUN: echo '#@ y=%t/y.pcm' >> %t/modmap
+// RUN: echo '#@x=%t/a.pcm ' >> %t/modmap
//
// RUN: %clang_cc1 -std=c++1z -fmodules-ts -I%t -fmodule-file=x=%t/a.pcm -verify %s \
// RUN:-DMODULE_NAME=x
@@ -33,6 +45,14 @@
// RUN: %clang_cc1 -std=c++1z -fmodules-ts -I%t -fmodule-file=z=%t/z.pcm -fmodule-file=y=%t/y.pcm -fmodule-file=x=%t/a.pcm -verify %s \
// RUN:-DMODULE_NAME=z
//
+//
+// RUN: %clang_cc1 -std=c++1z -fmodules-ts -I%t -fmodule-file-map=#@=%t/modmap -verify %s \
+// RUN:-DMODULE_NAME=x
+// RUN: %clang_cc1 -std=c++1z -fmodules-ts -I%t -fmodule-file-map=#@=%t/modmap -verify %s \
+// RUN:-DMODULE_NAME=y
+// RUN: %clang_cc1 -std=c++1z -fmodules-ts -I%t -fmodule-file-map=#@=%t/modmap -verify %s \
+// RUN:-DMODULE_NAME=z
+//
import MODULE_NAME;
Index: lib/Frontend/CompilerInvocation.cpp
===
--- lib/Frontend/CompilerInvocation.cpp
+++ lib/Frontend/CompilerInvocation.cpp
@@ -1528,8 +1528,89 @@
return P.str();
}
+// Read the mapping of module names to precompiled module files from a file.
+// The argument can include an optional line prefix ([]=), in
+// which case only lines that start with the prefix are considered (with the
+// prefix and the following whitespaces, if any, ignored).
+//
+// Each mapping entry should be in the same form as the -fmodule-file option
+// value (=) with leading/trailing whitespaces ignored.
+//
+static void LoadModuleFileMap(HeaderSearchOptions ,
+ DiagnosticsEngine ,
+ FileManager ,
+ StringRef Val,
+ const std::string ) {
+ // See if we have the prefix.
+ StringRef File;
+ StringRef Prefix;
+ if (Val.find('=') != StringRef::npos)
+ {
+auto Pair = Val.split('=');
+Prefix = Pair.first;
+File = Pair.second;
+if (Prefix.empty())
+{
+ Diags.Report(diag::err_drv_invalid_value) << Arg << Val;
+ return;
+}
+ }
+ else
+File = Val;
+
+ if (File.empty())
+ {
+Diags.Report(diag::err_drv_invalid_value) << Arg << Val;
+return;
+ }
+
+ auto Buf = FileMgr.getBufferForFile(File);
+ if (!Buf)
+ {
+Diags.Report(diag::err_cannot_open_file) << File << Buf.getError().message();
+return;
+ }
+
+ // Read the file line by line.
+ StringRef Str = Buf.get()->getBuffer();
+ for (size_t B(0), E(B); B < Str.size(); B = E + 1)
+ {
+E = Str.find_first_of(StringRef("\n\0", 2), B);
+
+if (E == StringRef::npos)
+ E = Str.size();
+else if (Str[E] == '\0')
+ break; // The file (or the rest of it) is binary, bail out.
