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

[Iain Sandoe via Phabricator via cfe-commits]

iains added inline comments.


================
Comment at: clang/lib/Sema/SemaModule.cpp:177
+  if (IsPartition) {
+    ModuleName += ":";
+    ModuleName += stringFromPath(Partition);
----------------
urnathan wrote:
> 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!)
(just to expand a little more)

the on-disk name needs to be chosen suitably for the platform and by the user and/or the build system.

When the FE chooses a default filename (which is done in building jobs, not in the Parser of course) it chooses one based on the source filename.  It would be most unfortunate if the Parser/Sema needed to understand platform filename rules.

When you do  'clang -module-file-info' (with the existing or updated version) you should see the module name as per the source code, the translation would only apply to the filename itself.

- to answer a later comment:

when we do -fmodule-file=A_B.pcm  and A_B.pcm contains a module named A:B the loader notices this pairing when it pre-loads the module, so that when we ask for "A:B" the loader already knows which on-disk file contains. it.

if we use the HeaderSearch mechanisms (when we want to figure out a module-name<=> filename pairing without loading the module) we can use -fmodule-name=A:B=A_B.pcm,

These mechanisms work today, but P1184 is a more flexible mechanism and avoids having  massive command lines with many -fmodule-file= cases.



================
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?");
----------------
urnathan wrote:
> 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.
that was what I expected.


================
Comment at: clang/lib/Sema/SemaModule.cpp:359
+
   std::string ModuleName;
+  if (IsPartition) {
----------------
ChuanqiXu wrote:
> I prefer to move the variable to the following block.
again the code is reorganised in 4/8


================
Comment at: clang/lib/Sema/SemaModule.cpp:433-451
+  if (NamePath.empty()) {
+    // If this was a header import, pad out with dummy locations.
+    // FIXME: Pass in and use the location of the header-name token in this
+    // case.
+    for (Module *ModCheck = Mod; ModCheck; ModCheck = ModCheck->Parent)
       IdentifierLocs.push_back(SourceLocation());
+  } else if (getLangOpts().CPlusPlusModules && !Mod->Parent) {
----------------
ChuanqiXu wrote:
> I don't find the reason to refactor here. It looks NFC and I think the original form is simpler.
it is not NFC:
1. we add the C++20 case (the name is a single identifier)
2. we avoid repeating the check for an empty Path (the first loop would effectively check that too).



================
Comment at: clang/test/Modules/cxx20-import-diagnostics-a.cpp:57
 
+module;
+
----------------
ChuanqiXu wrote:
> What if we don't add this? I think the original is good. So we should add a new test if we feel needed.
OK, probably it should have been in from the start - but we will also check these things in the tests taken from the standard, so I don't think it is important here; removed.


================
Comment at: clang/test/Modules/cxx20-partition-diagnostics-a.cpp:3-5
+// RUN: %clang_cc1 -std=c++20 -S -D TU=1 -x c++ %s -o /dev/null -verify
+
+// RUN: %clang_cc1 -std=c++20 -S -D TU=2 -x c++ %s -o /dev/null -verify
----------------
ChuanqiXu wrote:
> We could use `-fsyntax-only` instead of `-o /dev/null`
heh, I usually do this not sure why I did not here; changed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118586