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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
