[PATCH] D37299: [Modules] Add ability to specify module name to module file mapping in a file

[Boris Kolpackov via Phabricator via cfe-commits]

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 Kolpackov via Phabricator via cfe-commits]

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 Kolpackov via Phabricator via cfe-commits]

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 Kolpackov via Phabricator via cfe-commits]

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] D40443: [Modules TS] Make imports from an interface unit visible to its implementation units

[Boris Kolpackov via Phabricator via cfe-commits]

boris added a comment.

I don't think it will be wise for me to accept this revision since I can't 
claim to have good understanding of Clang's internal modules model. I think it 
will be wise to have Richard take a look.




Comment at: lib/Basic/Module.cpp:349
 
+  // Everything visible to the interface unit's global module fragment is
+  // visible to the interface unit.

This comment (not sure about the code)  sounds wrong to me: names from the 
interface unit before the module purview (I assume this is what "global module 
fragment" refers to) should not be visible in the implementation units.


https://reviews.llvm.org/D40443



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Boris Kolpackov via Phabricator via cfe-commits]

boris marked 2 inline comments as done.
boris added inline comments.



Comment at: lib/Frontend/CompilerInvocation.cpp:986
+if (Val.find('=') == StringRef::npos)
+  Opts.ExtraDeps.push_back(Val);
+  }

rsmith wrote:
> Does a module file specified via `-fmodule-file=foo=foo.pcm` get included in 
> the dep file if it is used? (If not, I'm happy for that to be fixed in a 
> separate change, but it does need to be fixed for depfile-based build 
> systems.)
No, currently it does not and neither a module file that is discovered via 
-fprebuilt-module-path. I think if we are doing this, then we should do for 
both cases. What do you think?


https://reviews.llvm.org/D35020



___
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 Kolpackov via Phabricator via cfe-commits]

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 &Opts,
+  DiagnosticsEngine &Diags,
+  FileManager &FileMgr,
+  StringRef Val,
+  const std::string &Arg) {
+  // 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

[PATCH] D37299: [Modules] Add ability to specify module name to module file mapping in a file

[Boris Kolpackov via Phabricator via cfe-commits]

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

[Boris Kolpackov via Phabricator via cfe-commits]

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 &Opts,
+  DiagnosticsEngine &Diags, FileManager &FileMgr,
+  StringRef Val, const std::string &Arg) {
+  // 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 &Opts, ArgList &Args,
-  const std::string &WorkingDir) {
+  DiagnosticsEngine &Diags,
+  FileManager &FileMgr) {
   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 &WorkingDir (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 &Opts, ArgList &Args,
-  FileManager &FileMgr,
   DiagnosticsEngine &Diags,
   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 &= Par

[PATCH] D37299: [Modules] Add ability to specify module name to module file mapping in a file

[Boris Kolpackov via Phabricator via cfe-commits]

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 Kolpackov via Phabricator via cfe-commits]

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 Kolpackov via Phabricator via cfe-commits]

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

[Boris Kolpackov via Phabricator via cfe-commits]

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 &Opts,
+  DiagnosticsEngine &Diags, FileManager &FileMgr,
+  StringRef Val, const std::string &Arg) {
+  // 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;
+
+

[PATCH] D37299: [Modules] Add ability to specify module name to module file mapping in a file

[Boris Kolpackov via Phabricator via cfe-commits]

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 &Opts,
+  DiagnosticsEngine &Diags, FileManager &FileMgr,
+  StringRef Val, const std::string &Arg) {
+  // 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) <<

[PATCH] D40443: [Modules TS] Make imports from an interface unit visible to its implementation units

[Boris Kolpackov via Phabricator via cfe-commits]

boris added a comment.

LGTM. Maybe it makes sense to also test that an unrelated translation unit that 
imports module 'test' sees neither 'a' nor 'b'.


https://reviews.llvm.org/D40443



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Boris Kolpackov via Phabricator via cfe-commits]

boris added a comment.

Ping.


https://reviews.llvm.org/D35020



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D35678: Omit sumbodule semantics for TS modules

[Boris Kolpackov via Phabricator via cfe-commits]

boris added a comment.

Ping.


https://reviews.llvm.org/D35678



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Boris Kolpackov via Phabricator via cfe-commits]

boris added a comment.

Ping.


https://reviews.llvm.org/D35020



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D35678: Omit sumbodule semantics for TS modules

[Boris Kolpackov via Phabricator via cfe-commits]

boris added a comment.

Ping.


https://reviews.llvm.org/D35678



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D35678: Omit sumbodule semantics for TS modules

[Boris Kolpackov via Phabricator via cfe-commits]

boris updated this revision to Diff 110875.
boris added a comment.

Richard, sorry for the last ping, somehow I missed your review.

I've uploaded a new revision with a test case (note that the issue is with 
'module a.b' not 'import a.b' but I have tested both for good measure).

My understanding of your comment is that the rest is ok for now (since there 
will probably be a redesign in this area). If, however, I misunderstood and you 
sill want to move the id flattening to the caller, let me know.


https://reviews.llvm.org/D35678

Files:
  lib/Frontend/CompilerInstance.cpp
  test/CXX/modules-ts/dcl.dcl/dcl.module/dcl.module.import/p1.cpp


Index: test/CXX/modules-ts/dcl.dcl/dcl.module/dcl.module.import/p1.cpp
===
--- test/CXX/modules-ts/dcl.dcl/dcl.module/dcl.module.import/p1.cpp
+++ test/CXX/modules-ts/dcl.dcl/dcl.module/dcl.module.import/p1.cpp
@@ -2,13 +2,17 @@
 // RUN: mkdir -p %t
 // RUN: echo 'export module x; export int a, b;' > %t/x.cppm
 // RUN: echo 'export module x.y; export int c;' > %t/x.y.cppm
+// RUN: echo 'export module a.b; export int d;' > %t/a.b.cppm
 //
 // 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/x.y.cppm -o %t/x.y.pcm
+// RUN: %clang_cc1 -std=c++1z -fmodules-ts -emit-module-interface %t/a.b.cppm 
-o %t/a.b.pcm
 //
-// RUN: %clang_cc1 -std=c++1z -fmodules-ts -I%t -fmodule-file=%t/x.y.pcm 
-verify %s \
+// RUN: %clang_cc1 -std=c++1z -fmodules-ts -I%t -fmodule-file=%t/x.y.pcm 
-fmodule-file=%t/a.b.pcm -verify %s \
 // RUN:-DMODULE_NAME=z
-// RUN: %clang_cc1 -std=c++1z -fmodules-ts -I%t -fmodule-file=%t/x.y.pcm 
-verify %s \
+// RUN: %clang_cc1 -std=c++1z -fmodules-ts -I%t -fmodule-file=%t/x.y.pcm 
-fmodule-file=%t/a.b.pcm -verify %s \
+// RUN:-DMODULE_NAME=a.b
+// RUN: %clang_cc1 -std=c++1z -fmodules-ts -I%t -fmodule-file=%t/x.y.pcm 
-fmodule-file=%t/a.b.pcm -verify %s \
 // RUN:-DMODULE_X -DMODULE_NAME=x
 
 module MODULE_NAME;
@@ -33,6 +37,7 @@
 import x [[blarg::noreturn]]; // expected-warning {{unknown attribute 
'noreturn' ignored}}
 
 import x.y;
+import a.b; // Does not imply existence of module a.
 import x.; // expected-error {{expected a module name after 'import'}}
 import .x; // expected-error {{expected a module name after 'import'}}
 
Index: lib/Frontend/CompilerInstance.cpp
===
--- lib/Frontend/CompilerInstance.cpp
+++ lib/Frontend/CompilerInstance.cpp
@@ -1601,7 +1601,22 @@
  Module::NameVisibilityKind Visibility,
  bool IsInclusionDirective) {
   // Determine what file we're searching from.
-  StringRef ModuleName = Path[0].first->getName();
+  // FIXME: Should we be deciding whether this is a submodule (here and
+  // below) based on -fmodules-ts or should we pass a flag and make the
+  // caller decide?
+  std::string ModuleName;
+  if (getLangOpts().ModulesTS) {
+// FIXME: Same code as Sema::ActOnModuleDecl() so there is probably a
+// better place/way to do this.
+for (auto &Piece : Path) {
+  if (!ModuleName.empty())
+ModuleName += ".";
+  ModuleName += Piece.first->getName();
+}
+  }
+  else
+ModuleName = Path[0].first->getName();
+
   SourceLocation ModuleNameLoc = Path[0].second;
 
   // If we've already handled this import, just return the cached result.
@@ -1816,7 +1831,7 @@
 
   // Verify that the rest of the module path actually corresponds to
   // a submodule.
-  if (Path.size() > 1) {
+  if (!getLangOpts().ModulesTS && Path.size() > 1) {
 for (unsigned I = 1, N = Path.size(); I != N; ++I) {
   StringRef Name = Path[I].first->getName();
   clang::Module *Sub = Module->findSubmodule(Name);


Index: test/CXX/modules-ts/dcl.dcl/dcl.module/dcl.module.import/p1.cpp
===
--- test/CXX/modules-ts/dcl.dcl/dcl.module/dcl.module.import/p1.cpp
+++ test/CXX/modules-ts/dcl.dcl/dcl.module/dcl.module.import/p1.cpp
@@ -2,13 +2,17 @@
 // RUN: mkdir -p %t
 // RUN: echo 'export module x; export int a, b;' > %t/x.cppm
 // RUN: echo 'export module x.y; export int c;' > %t/x.y.cppm
+// RUN: echo 'export module a.b; export int d;' > %t/a.b.cppm
 //
 // 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/x.y.cppm -o %t/x.y.pcm
+// RUN: %clang_cc1 -std=c++1z -fmodules-ts -emit-module-interface %t/a.b.cppm -o %t/a.b.pcm
 //
-// RUN: %clang_cc1 -std=c++1z -fmodules-ts -I%t -fmodule-file=%t/x.y.pcm -verify %s \
+// RUN: %clang_cc1 -std=c++1z -fmodules-ts -I%t -fmodule-file=%t/x.y.pcm -fmodule-file=%t/a.b.pcm -verify %s \
 // RUN:-DMODULE_NAME=z
-// RUN: %clang_cc1 -std=c++1z -fmodules-ts

[PATCH] D35678: Omit sumbodule semantics for TS modules

[Boris Kolpackov via Phabricator via cfe-commits]

boris added a comment.

In https://reviews.llvm.org/D35678#842891, @rsmith wrote:

> I'd still like the id flattening moved to the caller. [...] I'm fine with 
> that being done as a separate change after this one, though, if you'd prefer.


Yes, that would probably be easier.

> Do you need someone to commit this for you?

Yes, please, I don't have commit rights. Though if I were given them, I could 
do that myself (and it would also help with the other patch, I guess).


https://reviews.llvm.org/D35678



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Boris Kolpackov via Phabricator via cfe-commits]

boris marked 6 inline comments as done.
boris added a comment.

I've marked as "done" items that I have resolved in my local revision (not yet 
uploaded) and have added one comment for further feedback.




Comment at: lib/Frontend/CompilerInvocation.cpp:982
+StringRef Val = A->getValue();
+if (Val.find('=') == StringRef::npos)
+  Opts.ExtraDeps.push_back(Val);

rsmith wrote:
> 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.)
A couple of thoughts:

1. Using stat() is also not without issues: I may have an unrelated file with a 
name that matches the whole value -- this will be fun to debug.

2. GCC seems to have given up on trying to support paths with '=' since AFAIK, 
none of their key=value options (e.g., -fdebug-prefix-map) support any kind of 
escaping. I think the implicit assumption is that if you are using paths with 
'=' in them, then you've asked for it.

3. The '/' idea will work but will get a bit hairier if we also want to support 
Windows (will need to check for both slashes).

4. Another option is to reserve empty module name as a way to escape '=': 
-fmodule-file==foo=bar.pcm. Not backwards compatible (but neither is ./) and 
looks a bit weird but is the simplest to implement.

My preference order is (2), (4), (3), (1). Let me know which way you want to go.




https://reviews.llvm.org/D35020



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Boris Kolpackov via Phabricator via cfe-commits]

boris added inline comments.



Comment at: lib/Serialization/ASTReader.cpp:4145-4146
+// by-name lookup.
+if (Type == MK_PrebuiltModule || Type == MK_ExplicitModule)
+  ModuleMgr.registerPrebuilt(F);
 break;

rsmith wrote:
> 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.)
Yes, I was not entirely happy with this register prebuilt business so thanks 
for the hint. I've implemented this and all tests pass (both Clang's and my 
own). Though I cannot say I fully understand all the possible scenarios (like 
when can ASTFile be NULL).



> (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.)

Are you suggesting that we switch to storing module names instead of module 
files for all the module types in the offset map? If so, I think I've tried 
that at some point but it didn't go well (existing tests were failing if I 
remember correctly). I can try again if you think this should work. 




https://reviews.llvm.org/D35020



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Boris Kolpackov via Phabricator via cfe-commits]

boris updated this revision to Diff 111826.
boris added a comment.

New revision of the patch that I believe addresses all the issues except for 
the '=' escaping.


https://reviews.llvm.org/D35020

Files:
  docs/Modules.rst
  include/clang/Driver/Options.td
  include/clang/Lex/HeaderSearch.h
  include/clang/Lex/HeaderSearchOptions.h
  include/clang/Serialization/ASTReader.h
  include/clang/Serialization/ModuleManager.h
  lib/Driver/ToolChains/Clang.cpp
  lib/Frontend/CompilerInstance.cpp
  lib/Frontend/CompilerInvocation.cpp
  lib/Frontend/FrontendActions.cpp
  lib/Lex/HeaderSearch.cpp
  lib/Serialization/ASTReader.cpp
  lib/Serialization/ASTWriter.cpp
  lib/Serialization/GlobalModuleIndex.cpp
  lib/Serialization/ModuleManager.cpp
  test/CXX/modules-ts/basic/basic.search/module-import.cpp

Index: test/CXX/modules-ts/basic/basic.search/module-import.cpp
===
--- /dev/null
+++ test/CXX/modules-ts/basic/basic.search/module-import.cpp
@@ -0,0 +1,39 @@
+// Tests for imported module search.
+//
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// 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: %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
+//
+// RUN: %clang_cc1 -std=c++1z -fmodules-ts -I%t -fmodule-file=%t/x.pcm -verify %s \
+// RUN:-DMODULE_NAME=x
+// RUN: %clang_cc1 -std=c++1z -fmodules-ts -I%t -fmodule-file=%t/y.pcm -verify %s \
+// RUN:-DMODULE_NAME=y
+//
+// RUN: %clang_cc1 -std=c++1z -fmodules-ts -I%t -fmodule-file=x=%t/x.pcm -verify %s \
+// RUN:-DMODULE_NAME=x
+// RUN: %clang_cc1 -std=c++1z -fmodules-ts -I%t -fmodule-file=y=%t/y.pcm -verify %s \
+// RUN:-DMODULE_NAME=y
+//
+// RUN: mv %t/x.pcm %t/a.pcm
+//
+// RUN: %clang_cc1 -std=c++1z -fmodules-ts -I%t -fmodule-file=x=%t/a.pcm -verify %s \
+// RUN:-DMODULE_NAME=x
+// RUN: %clang_cc1 -std=c++1z -fmodules-ts -I%t -fmodule-file=%t/y.pcm -fmodule-file=x=%t/a.pcm -verify %s \
+// RUN:-DMODULE_NAME=y
+// RUN: %clang_cc1 -std=c++1z -fmodules-ts -I%t -fmodule-file=y=%t/y.pcm -fmodule-file=x=%t/a.pcm -verify %s \
+// RUN:-DMODULE_NAME=y
+//
+// RUN: %clang_cc1 -std=c++1z -fmodules-ts -emit-module-interface -fmodule-file=y=%t/y.pcm -fmodule-file=x=%t/a.pcm %t/z.cppm -o %t/z.pcm
+//
+// 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
+//
+
+import MODULE_NAME;
+
+// expected-no-diagnostics
Index: lib/Serialization/ModuleManager.cpp
===
--- lib/Serialization/ModuleManager.cpp
+++ lib/Serialization/ModuleManager.cpp
@@ -28,15 +28,23 @@
 using namespace clang;
 using namespace serialization;
 
-ModuleFile *ModuleManager::lookup(StringRef Name) const {
+ModuleFile *ModuleManager::lookupByFileName(StringRef Name) const {
   const FileEntry *Entry = FileMgr.getFile(Name, /*openFile=*/false,
/*cacheFailure=*/false);
   if (Entry)
 return lookup(Entry);
 
   return nullptr;
 }
 
+ModuleFile *ModuleManager::lookupByModuleName(StringRef Name) const {
+  if (const Module *Mod = HeaderSearchInfo.getModuleMap().findModule(Name))
+if (const FileEntry *File = Mod->getASTFile())
+  return lookup(File);
+
+  return nullptr;
+}
+
 ModuleFile *ModuleManager::lookup(const FileEntry *File) const {
   auto Known = Modules.find(File);
   if (Known == Modules.end())
@@ -306,9 +314,11 @@
 }
 
 ModuleManager::ModuleManager(FileManager &FileMgr, MemoryBufferCache &PCMCache,
- const PCHContainerReader &PCHContainerRdr)
+ const PCHContainerReader &PCHContainerRdr,
+ const HeaderSearch& HeaderSearchInfo)
 : FileMgr(FileMgr), PCMCache(&PCMCache), PCHContainerRdr(PCHContainerRdr),
-  GlobalIndex(), FirstVisitState(nullptr) {}
+  HeaderSearchInfo (HeaderSearchInfo), GlobalIndex(),
+  FirstVisitState(nullptr) {}
 
 ModuleManager::~ModuleManager() { delete FirstVisitState; }
 
Index: lib/Serialization/GlobalModuleIndex.cpp
===
--- lib/Serialization/GlobalModuleIndex.cpp
+++ lib/Serialization/GlobalModuleIndex.cpp
@@ -619,6 +619,10 @@
   (uint32_t)Record[Idx++], (uint32_t)Record[Idx++],
   (uint32_t)Record[Idx++]}}};
 
+// Skip the module name (currently this is only used for prebuilt
+// modules while here we are only dealing with cached).
+Idx += Record[Idx] + 1;
+
 // Retrieve the imported file name.
 unsigned Length 

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

[Boris Kolpackov via Phabricator via cfe-commits]

boris added a comment.

Ping.


https://reviews.llvm.org/D35020



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D134267: [C++] [Modules] Support one phase compilation model for named modules

[Boris Kolpackov via Phabricator via cfe-commits]

boris added a comment.

Just to add a few points on the module mapper discussion: overall, the current 
view is that there are two ways to handle C++20 modules from the build system's 
POV: (1) pre-scan the codebase to figure out what is what and who depends on 
whom ahead of compilation and (2) module mapper where the compiler and the 
build system discover this information interactively during compilation. 
Naturally, both approaches have advantages and disadvantages.

The major issue with the pre-scan is that there needs to be a clear boundary 
where the codebase ends. This can be an acceptable requirement for 
monorepo-style projects but not for multirepo. Even for monorepo, there are 
often external dependencies (C/C++ standard library, maybe system libraries 
like OpenSSL) that require special considerations. There is also some concern 
for the performance of the pre-scan on larger codebases. On the other hand, 
pre-scan is likely to be easier to integrate with legacy and meta-build systems 
like CMake.

The major issue with the module mapper is the need for deeper integration with 
the build system as well as the overall complexity of the client/server 
architecture. There is also some concern for the resource consumption since the 
mapper approach may need to spawn nested compiler invocations to build missing 
BMIs. The main advantage of the module mapper is probably the fact that the 
information comes from the source (the compiler) and if/when necessary, which 
sidesteps the whole issue of doing too little or too much pre-scanning (for 
example, due to imprecise boundaries, some discrepancies in the compiler 
options, etc).

GCC implements the mapper approach (with the reusable parts factored into 
libcody) and we successfully use that in build2. We would definitely like to 
see the module mapper supported by Clang, ideally with the GCC's interface (so 
that we could use the existing mapper for both). In the build2's case 
specifically, pre-scan is not a viable option since it's a multirepo-first 
build system.

Also, a comment on the earlier point made:

> I do not believe that the client-sever build system model (a.k.a. P1184 
>  / module mapper) is quite the same thing as 
> implicit modules

Strongly agree. The key difference here is that with the mapper there is a 
single entity (the build system) that decides which things need to be (re)built 
and where rather than multiple compiler instances trying to come up with 
something consistent.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134267/new/

https://reviews.llvm.org/D134267

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D134267: [C++] [Modules] Support one phase compilation model for named modules

[Boris Kolpackov via Phabricator via cfe-commits]

boris added a comment.

> For example, my experimental support for P1689 
>  is at: [...]. The implementation looks 
> relatively trivial to me. The simplicity is pretty important.

Two points:

1. It's worth considering the simplicity of the overall system (compiler + 
build system + user project) rather than just the compiler. I hope my previous 
comment highlighted some of the complexity that the overall system must deal 
with in the pre-scan approach with a lot of it potentially ending up on the 
user's plate.

2. I haven't looked at the code, but there are some complex problems in this 
area as highlighted by this MSVC bug: 
https://developercommunity.visualstudio.com/t/scanDependencies-does-not-take-into-acc/10029154
 I believe you may have it easier because reportedly Richard & friends have 
already implemented the necessary header import isolation semantics.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134267/new/

https://reviews.llvm.org/D134267

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://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 Kolpackov via Phabricator via cfe-commits]

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 Kolpackov via Phabricator via cfe-commits]

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 Kolpackov via Phabricator via cfe-commits]

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

[Boris Kolpackov via Phabricator via cfe-commits]

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] D40270: [Modules TS] Added re-export support

[Boris Kolpackov via Phabricator via cfe-commits]

boris added a comment.

LGTM. Will be happy to also run our re-export tests in build2 once this is 
merged.


https://reviews.llvm.org/D40270



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D40270: [Modules TS] Added re-export support

[Boris Kolpackov via Phabricator via cfe-commits]

boris added a comment.

All our tests pass as well. Thanks for your work!


Repository:
  rL LLVM

https://reviews.llvm.org/D40270



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Boris Kolpackov via Phabricator via cfe-commits]

boris created this revision.

Extend the -fmodule-file option to support the [=] value format.
If the name is omitted, then the old semantics is preserved (the module file
is loaded whether needed or not). If the name is specified, then the mapping
is treated as just another prebuilt module search mechanism, similar to
-fprebuilt-module-path, and the module file is only loaded if actually
used (e.g., via import). With one exception: this mapping also overrides
module file references embedded in other modules (which can be useful if
module files are moved/renamed as often happens during remote compilation).

This override semantics requires some extra work: we now store the module
name in addition to the file name in the serialized AST representation as
well as keep track of already loaded prebuilt modules by-name in addition
to by-file.

Patch by Boris Kolpackov

---

Additional notes:

1. Based on this mailing list discussion:

  http://lists.llvm.org/pipermail/cfe-dev/2017-June/054431.html

2. The need to change the serialized AST representation was a bit more than 
what I hoped for my first patch but based on this FIXME comment I recon this is 
moving in right direction:

  ASTReader::ReadModuleOffsetMap():

  // FIXME: Looking up dependency modules by filename is horrible.

  So now, at least for prebuilt modules, we look up by the module name.

3. Overloading -fmodule-file for this admittedly fairly different semantics 
might look like a bad idea and source of some unnecessary complexity. The 
problem with a separate option approach is the difficulty of finding a decent 
name that is not already used (e.g., -fmodule-map is out because of 
-fmodule-map-file; more details in the mailing list thread). But I am still 
open to changing this to a separate option if there are strong feelings (and 
good name suggestions ;-)).

4. I plan to implement a companion option that will read this mapping from a 
file. I will submit it as a separate patch once the general validity of the 
approach is confirmed.


https://reviews.llvm.org/D35020

Files:
  docs/ClangCommandLineReference.rst
  docs/Modules.rst
  include/clang/Driver/Options.td
  include/clang/Lex/HeaderSearch.h
  include/clang/Lex/HeaderSearchOptions.h
  include/clang/Serialization/ASTReader.h
  include/clang/Serialization/ModuleManager.h
  lib/Driver/ToolChains/Clang.cpp
  lib/Frontend/CompilerInstance.cpp
  lib/Frontend/CompilerInvocation.cpp
  lib/Frontend/FrontendActions.cpp
  lib/Lex/HeaderSearch.cpp
  lib/Serialization/ASTReader.cpp
  lib/Serialization/ASTWriter.cpp
  lib/Serialization/GlobalModuleIndex.cpp
  lib/Serialization/ModuleManager.cpp
  test/CXX/modules-ts/basic/basic.search/module-import.cpp

Index: test/CXX/modules-ts/basic/basic.search/module-import.cpp
===
--- /dev/null
+++ test/CXX/modules-ts/basic/basic.search/module-import.cpp
@@ -0,0 +1,39 @@
+// Tests for imported module search.
+//
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// 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: %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
+//
+// RUN: %clang_cc1 -std=c++1z -fmodules-ts -I%t -fmodule-file=%t/x.pcm -verify %s \
+// RUN:-DMODULE_NAME=x
+// RUN: %clang_cc1 -std=c++1z -fmodules-ts -I%t -fmodule-file=%t/y.pcm -verify %s \
+// RUN:-DMODULE_NAME=y
+//
+// RUN: %clang_cc1 -std=c++1z -fmodules-ts -I%t -fmodule-file=x=%t/x.pcm -verify %s \
+// RUN:-DMODULE_NAME=x
+// RUN: %clang_cc1 -std=c++1z -fmodules-ts -I%t -fmodule-file=y=%t/y.pcm -verify %s \
+// RUN:-DMODULE_NAME=y
+//
+// RUN: mv %t/x.pcm %t/a.pcm
+//
+// RUN: %clang_cc1 -std=c++1z -fmodules-ts -I%t -fmodule-file=x=%t/a.pcm -verify %s \
+// RUN:-DMODULE_NAME=x
+// RUN: %clang_cc1 -std=c++1z -fmodules-ts -I%t -fmodule-file=%t/y.pcm -fmodule-file=x=%t/a.pcm -verify %s \
+// RUN:-DMODULE_NAME=y
+// RUN: %clang_cc1 -std=c++1z -fmodules-ts -I%t -fmodule-file=y=%t/y.pcm -fmodule-file=x=%t/a.pcm -verify %s \
+// RUN:-DMODULE_NAME=y
+//
+// RUN: %clang_cc1 -std=c++1z -fmodules-ts -emit-module-interface -fmodule-file=y=%t/y.pcm -fmodule-file=x=%t/a.pcm %t/z.cppm -o %t/z.pcm
+//
+// 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
+//
+
+import MODULE_NAME;
+
+// expected-no-diagnostics
Index: lib/Serialization/ModuleManager.cpp
===
--- lib/Serialization/ModuleManager.cpp
+++ lib/Serialization/ModuleManager.cpp
@@ -45,6 +45,14 @@
   return Known->second;
 }
 
+ModuleFile *ModuleM

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

[Boris Kolpackov via Phabricator via cfe-commits]

boris updated this revision to Diff 106041.
boris added a comment.

Rebase on latest HEAD.


https://reviews.llvm.org/D35020

Files:
  docs/ClangCommandLineReference.rst
  docs/Modules.rst
  include/clang/Driver/Options.td
  include/clang/Lex/HeaderSearch.h
  include/clang/Lex/HeaderSearchOptions.h
  include/clang/Serialization/ASTReader.h
  include/clang/Serialization/ModuleManager.h
  lib/Driver/ToolChains/Clang.cpp
  lib/Frontend/CompilerInstance.cpp
  lib/Frontend/CompilerInvocation.cpp
  lib/Frontend/FrontendActions.cpp
  lib/Lex/HeaderSearch.cpp
  lib/Serialization/ASTReader.cpp
  lib/Serialization/ASTWriter.cpp
  lib/Serialization/GlobalModuleIndex.cpp
  lib/Serialization/ModuleManager.cpp
  test/CXX/modules-ts/basic/basic.search/module-import.cpp

Index: test/CXX/modules-ts/basic/basic.search/module-import.cpp
===
--- /dev/null
+++ test/CXX/modules-ts/basic/basic.search/module-import.cpp
@@ -0,0 +1,39 @@
+// Tests for imported module search.
+//
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// 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: %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
+//
+// RUN: %clang_cc1 -std=c++1z -fmodules-ts -I%t -fmodule-file=%t/x.pcm -verify %s \
+// RUN:-DMODULE_NAME=x
+// RUN: %clang_cc1 -std=c++1z -fmodules-ts -I%t -fmodule-file=%t/y.pcm -verify %s \
+// RUN:-DMODULE_NAME=y
+//
+// RUN: %clang_cc1 -std=c++1z -fmodules-ts -I%t -fmodule-file=x=%t/x.pcm -verify %s \
+// RUN:-DMODULE_NAME=x
+// RUN: %clang_cc1 -std=c++1z -fmodules-ts -I%t -fmodule-file=y=%t/y.pcm -verify %s \
+// RUN:-DMODULE_NAME=y
+//
+// RUN: mv %t/x.pcm %t/a.pcm
+//
+// RUN: %clang_cc1 -std=c++1z -fmodules-ts -I%t -fmodule-file=x=%t/a.pcm -verify %s \
+// RUN:-DMODULE_NAME=x
+// RUN: %clang_cc1 -std=c++1z -fmodules-ts -I%t -fmodule-file=%t/y.pcm -fmodule-file=x=%t/a.pcm -verify %s \
+// RUN:-DMODULE_NAME=y
+// RUN: %clang_cc1 -std=c++1z -fmodules-ts -I%t -fmodule-file=y=%t/y.pcm -fmodule-file=x=%t/a.pcm -verify %s \
+// RUN:-DMODULE_NAME=y
+//
+// RUN: %clang_cc1 -std=c++1z -fmodules-ts -emit-module-interface -fmodule-file=y=%t/y.pcm -fmodule-file=x=%t/a.pcm %t/z.cppm -o %t/z.pcm
+//
+// 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
+//
+
+import MODULE_NAME;
+
+// expected-no-diagnostics
Index: lib/Serialization/ModuleManager.cpp
===
--- lib/Serialization/ModuleManager.cpp
+++ lib/Serialization/ModuleManager.cpp
@@ -45,6 +45,14 @@
   return Known->second;
 }
 
+ModuleFile *ModuleManager::lookupPrebuilt(StringRef Name) const {
+  auto Known = PrebuiltModules.find(Name);
+  if (Known == PrebuiltModules.end())
+return nullptr;
+
+  return Known->second;
+}
+
 std::unique_ptr
 ModuleManager::lookupBuffer(StringRef Name) {
   const FileEntry *Entry = FileMgr.getFile(Name, /*openFile=*/false,
@@ -192,6 +200,11 @@
   return NewlyLoaded;
 }
 
+void ModuleManager::registerPrebuilt (ModuleFile &M) {
+  assert (!M.ModuleName.empty());
+  PrebuiltModules[M.ModuleName] = &M;
+}
+
 void ModuleManager::removeModules(
 ModuleIterator First,
 llvm::SmallPtrSetImpl &LoadedSuccessfully,
@@ -232,6 +245,8 @@
   // Delete the modules and erase them from the various structures.
   for (ModuleIterator victim = First; victim != Last; ++victim) {
 Modules.erase(victim->File);
+if (!victim->ModuleName.empty())
+  PrebuiltModules.erase(victim->ModuleName);
 
 if (modMap) {
   StringRef ModuleName = victim->ModuleName;
Index: lib/Serialization/GlobalModuleIndex.cpp
===
--- lib/Serialization/GlobalModuleIndex.cpp
+++ lib/Serialization/GlobalModuleIndex.cpp
@@ -619,6 +619,10 @@
   (uint32_t)Record[Idx++], (uint32_t)Record[Idx++],
   (uint32_t)Record[Idx++]}}};
 
+// Skip the module name (currently this is only used for prebuilt
+// modules while here we are only dealing with cached).
+Idx += Record[Idx] + 1;
+
 // Retrieve the imported file name.
 unsigned Length = Record[Idx++];
 SmallString<128> ImportedFile(Record.begin() + Idx,
Index: lib/Serialization/ASTWriter.cpp
===
--- lib/Serialization/ASTWriter.cpp
+++ lib/Serialization/ASTWriter.cpp
@@ -1505,6 +1505,7 @@
   for (auto I : M.Signature)
 Record.push_back(I);
 
+  AddString(M.ModuleName, Record);
   AddPath(M.FileName, Rec

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

[Boris Kolpackov via Phabricator via cfe-commits]

boris added a comment.

Ping.


https://reviews.llvm.org/D35020



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Boris Kolpackov via Phabricator via cfe-commits]

boris updated this revision to Diff 107017.
boris added a comment.

Rebase on latest HEAD.


https://reviews.llvm.org/D35020

Files:
  docs/ClangCommandLineReference.rst
  docs/Modules.rst
  include/clang/Driver/Options.td
  include/clang/Lex/HeaderSearch.h
  include/clang/Lex/HeaderSearchOptions.h
  include/clang/Serialization/ASTReader.h
  include/clang/Serialization/ModuleManager.h
  lib/Driver/ToolChains/Clang.cpp
  lib/Frontend/CompilerInstance.cpp
  lib/Frontend/CompilerInvocation.cpp
  lib/Frontend/FrontendActions.cpp
  lib/Lex/HeaderSearch.cpp
  lib/Serialization/ASTReader.cpp
  lib/Serialization/ASTWriter.cpp
  lib/Serialization/GlobalModuleIndex.cpp
  lib/Serialization/ModuleManager.cpp
  test/CXX/modules-ts/basic/basic.search/module-import.cpp

Index: test/CXX/modules-ts/basic/basic.search/module-import.cpp
===
--- /dev/null
+++ test/CXX/modules-ts/basic/basic.search/module-import.cpp
@@ -0,0 +1,39 @@
+// Tests for imported module search.
+//
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// 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: %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
+//
+// RUN: %clang_cc1 -std=c++1z -fmodules-ts -I%t -fmodule-file=%t/x.pcm -verify %s \
+// RUN:-DMODULE_NAME=x
+// RUN: %clang_cc1 -std=c++1z -fmodules-ts -I%t -fmodule-file=%t/y.pcm -verify %s \
+// RUN:-DMODULE_NAME=y
+//
+// RUN: %clang_cc1 -std=c++1z -fmodules-ts -I%t -fmodule-file=x=%t/x.pcm -verify %s \
+// RUN:-DMODULE_NAME=x
+// RUN: %clang_cc1 -std=c++1z -fmodules-ts -I%t -fmodule-file=y=%t/y.pcm -verify %s \
+// RUN:-DMODULE_NAME=y
+//
+// RUN: mv %t/x.pcm %t/a.pcm
+//
+// RUN: %clang_cc1 -std=c++1z -fmodules-ts -I%t -fmodule-file=x=%t/a.pcm -verify %s \
+// RUN:-DMODULE_NAME=x
+// RUN: %clang_cc1 -std=c++1z -fmodules-ts -I%t -fmodule-file=%t/y.pcm -fmodule-file=x=%t/a.pcm -verify %s \
+// RUN:-DMODULE_NAME=y
+// RUN: %clang_cc1 -std=c++1z -fmodules-ts -I%t -fmodule-file=y=%t/y.pcm -fmodule-file=x=%t/a.pcm -verify %s \
+// RUN:-DMODULE_NAME=y
+//
+// RUN: %clang_cc1 -std=c++1z -fmodules-ts -emit-module-interface -fmodule-file=y=%t/y.pcm -fmodule-file=x=%t/a.pcm %t/z.cppm -o %t/z.pcm
+//
+// 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
+//
+
+import MODULE_NAME;
+
+// expected-no-diagnostics
Index: lib/Serialization/ModuleManager.cpp
===
--- lib/Serialization/ModuleManager.cpp
+++ lib/Serialization/ModuleManager.cpp
@@ -45,6 +45,14 @@
   return Known->second;
 }
 
+ModuleFile *ModuleManager::lookupPrebuilt(StringRef Name) const {
+  auto Known = PrebuiltModules.find(Name);
+  if (Known == PrebuiltModules.end())
+return nullptr;
+
+  return Known->second;
+}
+
 std::unique_ptr
 ModuleManager::lookupBuffer(StringRef Name) {
   const FileEntry *Entry = FileMgr.getFile(Name, /*openFile=*/false,
@@ -192,6 +200,11 @@
   return NewlyLoaded;
 }
 
+void ModuleManager::registerPrebuilt (ModuleFile &M) {
+  assert (!M.ModuleName.empty());
+  PrebuiltModules[M.ModuleName] = &M;
+}
+
 void ModuleManager::removeModules(
 ModuleIterator First,
 llvm::SmallPtrSetImpl &LoadedSuccessfully,
@@ -232,6 +245,8 @@
   // Delete the modules and erase them from the various structures.
   for (ModuleIterator victim = First; victim != Last; ++victim) {
 Modules.erase(victim->File);
+if (!victim->ModuleName.empty())
+  PrebuiltModules.erase(victim->ModuleName);
 
 if (modMap) {
   StringRef ModuleName = victim->ModuleName;
Index: lib/Serialization/GlobalModuleIndex.cpp
===
--- lib/Serialization/GlobalModuleIndex.cpp
+++ lib/Serialization/GlobalModuleIndex.cpp
@@ -619,6 +619,10 @@
   (uint32_t)Record[Idx++], (uint32_t)Record[Idx++],
   (uint32_t)Record[Idx++]}}};
 
+// Skip the module name (currently this is only used for prebuilt
+// modules while here we are only dealing with cached).
+Idx += Record[Idx] + 1;
+
 // Retrieve the imported file name.
 unsigned Length = Record[Idx++];
 SmallString<128> ImportedFile(Record.begin() + Idx,
Index: lib/Serialization/ASTWriter.cpp
===
--- lib/Serialization/ASTWriter.cpp
+++ lib/Serialization/ASTWriter.cpp
@@ -1505,6 +1505,7 @@
   for (auto I : M.Signature)
 Record.push_back(I);
 
+  AddString(M.ModuleName, Record);
   AddPath(M.FileName, Rec

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

[Boris Kolpackov via Phabricator via cfe-commits]

boris added a comment.

Ping.

I am holding off on proposing the same functionality to GCC because I want to 
make sure the command line interface is the same for both compilers (GCC has 
less baggage in this area, option-name-wise). So confirming that at least the 
naming/semantics are acceptable would be very helpful.


https://reviews.llvm.org/D35020



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D35678: Omit sumbodule semantics for TS modules

[Boris Kolpackov via Phabricator via cfe-commits]

boris created this revision.

If a TS module name has more than one component (e.g., foo.bar) then we 
erroneously activate the submodule semantics when encountering a module 
declaration in the module implementation unit (e.g., module foo.bar;).


https://reviews.llvm.org/D35678

Files:
  lib/Frontend/CompilerInstance.cpp


Index: lib/Frontend/CompilerInstance.cpp
===
--- lib/Frontend/CompilerInstance.cpp
+++ lib/Frontend/CompilerInstance.cpp
@@ -1595,7 +1595,22 @@
  Module::NameVisibilityKind Visibility,
  bool IsInclusionDirective) {
   // Determine what file we're searching from.
-  StringRef ModuleName = Path[0].first->getName();
+  // FIXME: Should we be deciding whether this is a submodule (here and
+  // below) based on -fmodules-ts or should we pass a flag and make the
+  // caller decide?
+  std::string ModuleName;
+  if (getLangOpts().ModulesTS) {
+// FIXME: Same code as Sema::ActOnModuleDecl() so there is probably a
+// better place/way to do this.
+for (auto &Piece : Path) {
+  if (!ModuleName.empty())
+ModuleName += ".";
+  ModuleName += Piece.first->getName();
+}
+  }
+  else
+ModuleName = Path[0].first->getName();
+
   SourceLocation ModuleNameLoc = Path[0].second;
 
   // If we've already handled this import, just return the cached result.
@@ -1810,7 +1825,7 @@
 
   // Verify that the rest of the module path actually corresponds to
   // a submodule.
-  if (Path.size() > 1) {
+  if (!getLangOpts().ModulesTS && Path.size() > 1) {
 for (unsigned I = 1, N = Path.size(); I != N; ++I) {
   StringRef Name = Path[I].first->getName();
   clang::Module *Sub = Module->findSubmodule(Name);


Index: lib/Frontend/CompilerInstance.cpp
===
--- lib/Frontend/CompilerInstance.cpp
+++ lib/Frontend/CompilerInstance.cpp
@@ -1595,7 +1595,22 @@
  Module::NameVisibilityKind Visibility,
  bool IsInclusionDirective) {
   // Determine what file we're searching from.
-  StringRef ModuleName = Path[0].first->getName();
+  // FIXME: Should we be deciding whether this is a submodule (here and
+  // below) based on -fmodules-ts or should we pass a flag and make the
+  // caller decide?
+  std::string ModuleName;
+  if (getLangOpts().ModulesTS) {
+// FIXME: Same code as Sema::ActOnModuleDecl() so there is probably a
+// better place/way to do this.
+for (auto &Piece : Path) {
+  if (!ModuleName.empty())
+ModuleName += ".";
+  ModuleName += Piece.first->getName();
+}
+  }
+  else
+ModuleName = Path[0].first->getName();
+
   SourceLocation ModuleNameLoc = Path[0].second;
 
   // If we've already handled this import, just return the cached result.
@@ -1810,7 +1825,7 @@
 
   // Verify that the rest of the module path actually corresponds to
   // a submodule.
-  if (Path.size() > 1) {
+  if (!getLangOpts().ModulesTS && Path.size() > 1) {
 for (unsigned I = 1, N = Path.size(); I != N; ++I) {
   StringRef Name = Path[I].first->getName();
   clang::Module *Sub = Module->findSubmodule(Name);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Boris Kolpackov via Phabricator via cfe-commits]

boris added a comment.

Ping.

FWIW, I went ahead and implemented this functionality in GCC. It has been 
merged into its c++-modules branch.


https://reviews.llvm.org/D35020



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D35678: Omit sumbodule semantics for TS modules

[Boris Kolpackov via Phabricator via cfe-commits]

boris added a comment.

Ping.


https://reviews.llvm.org/D35678



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits