[PATCH] D35678: Omit sumbodule semantics for TS modules

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL312007: [modules-ts] Omit submodule semantics for TS modules 
(authored by borisk).

Changed prior to commit:
  https://reviews.llvm.org/D35678?vs=110875&id=113098#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D35678

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


Index: cfe/trunk/test/CXX/modules-ts/dcl.dcl/dcl.module/dcl.module.import/p1.cpp
===
--- cfe/trunk/test/CXX/modules-ts/dcl.dcl/dcl.module/dcl.module.import/p1.cpp
+++ cfe/trunk/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: cfe/trunk/lib/Frontend/CompilerInstance.cpp
===
--- cfe/trunk/lib/Frontend/CompilerInstance.cpp
+++ cfe/trunk/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: cfe/trunk/test/CXX/modules-ts/dcl.dcl/dcl.module/dcl.module.import/p1.cpp
===
--- cfe/trunk/test/CXX/modules-ts/dcl.dcl/dcl.module/dcl.module.import/p1.cpp
+++ cfe/trunk/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.

[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] D35678: Omit sumbodule semantics for TS modules

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.

In https://reviews.llvm.org/D35678#840092, @boris wrote:

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


I'd still like the id flattening moved to the caller. I don't know when or if 
the redesign will ever happen, so we should aim for the state of the codebase 
to be consistent in the interim (which might be forever). I'm fine with that 
being done as a separate change after this one, though, if you'd prefer.

Do you need someone to commit this for you?


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.

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

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

This will need a test case.




Comment at: lib/Frontend/CompilerInstance.cpp:1598-1612
+  // 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.

The strategy we've been using so far is this: components after the first in a 
module path name submodules (visibility groups within a module). Modules TS 
modules use only the first name component, which may contain periods if the 
module name contains periods.

So the burden should be on the code that produces a `ModuleIdPath` to do this 
flattening, not on consumers of the `ModuleIdPath`.

(FWIW, I think we may want to consider this later, so that `import foo.bar;` 
could import either the Modules TS module `foo.bar`, or the submodule `bar` of 
the module map module `foo`, but that will take some fundamental restructuring 
of how we model `Module` objects and their hierarchy.)


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 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 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 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