[PATCH] D118586: [C++20][Modules][3/8] Initial handling for module partitions.

[Nathan Sidwell via Phabricator via cfe-commits]

urnathan added inline comments.


================
Comment at: clang/lib/Sema/SemaModule.cpp:177
+  if (IsPartition) {
+    ModuleName += ":";
+    ModuleName += stringFromPath(Partition);
----------------
iains wrote:
> ChuanqiXu wrote:
> > I chose '-' in my implementation since here ModuleName refers to the name in filesystem, right? And I remember '-' is the choice of GCC. So let's keep consistency.
> This is not my understanding.
> 
> ModuleName is the "user-facing" name of the module that is described in the source, and we use this in diagnostics etc.
> 
> The translation of that name to an on-disk name can be arbitrary and there are several mechanisms in clang already (e.g. -fmodule-file=A:B=foo.pcm) the module loader uses these to find the module on the basis of its user-facing name where required.
> 
> When we have P1184, it is even more important that the interface is supplied with the name that the user will put into the source code.
> 
> 
I agree with Iain, we should use ':' is module names here.  When mapping a named module to the file system some translation is needed because ':' is not permitted in file names on some filesystems (you know the ones!)


================
Comment at: clang/lib/Sema/SemaModule.cpp:350
+  bool IsPartition = !Partition.empty();
+  bool Cxx20Mode = getLangOpts().CPlusPlusModules || getLangOpts().ModulesTS;
+  assert((!IsPartition || Cxx20Mode) && "partition seen in non-C++20 code?");
----------------
iains wrote:
> ChuanqiXu wrote:
> > I prefer to remove the support for getLangOpts().ModulesTS on the fly.
> agreed in comments, but I think it is outside of my remit to remove the functionality?
We can't just drop the -fmodules-ts option.  I think its semantics should be[come] the same as CPlusPlusModules.  That's not a change for this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118586