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

[Iain Sandoe via Phabricator via cfe-commits]

iains marked 7 inline comments as done.
iains added a comment.

some comments remain to be addressed - this update addressed Nathan's primarily - but also cover some of Chuanqi's



================
Comment at: clang/include/clang/Sema/Sema.h:2989
   /// \param ImportLoc The location of the 'import' keyword.
-  /// \param Path The module access path.
+  /// \param NamePath The module toplevel name as an access path.
+  /// \param Partition The module partition name as an access path.
----------------
ChuanqiXu wrote:
> urnathan wrote:
> > Is `NamePath` really a better name?  You've not consistently changed all `Path`'s to this, and it doesn;t strike me as particularly mnemonic.
> I use `Path` in my implementation.
I\ve reverted my changes - leaving the variable as "Path"

This is a problem with the code serving two masters ...

... for clang hierachical modules "Path" makes some sense
... for  C++20 not really - it is not a pathname (confused me when first reading the code).

however "NamedModuleNameAsPath" is too long ;)



================
Comment at: clang/lib/Parse/Parser.cpp:2394
 /// Parse a module import declaration. This is essentially the same for
-/// Objective-C and the C++ Modules TS, except for the leading '@' (in ObjC)
+/// Objective-C and the C++20/Modules TS, except for the leading '@' (in ObjC)
 /// and the trailing optional attributes (in C++).
----------------
urnathan wrote:
> perhaps now we should just remove modules-ts references as drive-by cleanups?
will do this generally now .. given consensus amongst active reviewers.


================
Comment at: clang/lib/Sema/SemaModule.cpp:177
+  if (IsPartition) {
+    ModuleName += ":";
+    ModuleName += stringFromPath(Partition);
----------------
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.




================
Comment at: clang/lib/Sema/SemaModule.cpp:224-225
+    if (IsPartition) {
+      // Create an interface, but note that it is an implementation
+      // unit.
       Mod = Map.createModuleForInterfaceUnit(ModuleLoc, ModuleName,
----------------
ChuanqiXu wrote:
> I prefer to add more words here to tell why we create an interface unit but the above condition is `ModuleDeclKind::Implementation`. 
I am sorry, perhaps 4/8 should have been squashed with this patch - but this was getting quite large anyway - please see 4/8 for reorganisation of this code.


================
Comment at: clang/lib/Sema/SemaModule.cpp:260
+  ModuleScopes.back().ModuleInterface =
+      (MDK != ModuleDeclKind::Implementation || IsPartition);
+  ModuleScopes.back().IsPartition = IsPartition;
----------------
ChuanqiXu wrote:
> How about: `Mod->Kind != Module::ModulePartitionImplementation`.
please see 4/8


================
Comment at: clang/lib/Sema/SemaModule.cpp:346-347
                                    SourceLocation ImportLoc,
-                                   ModuleIdPath Path) {
-  // Flatten the module path for a C++20 or Modules TS module name.
+                                   ModuleIdPath NamePath,
+                                   ModuleIdPath Partition) {
+
----------------
ChuanqiXu wrote:
> I think we could change the signature of the lat 2 fields to 
> ```
> ModuleIdPath Path,
> bool IsPartition)
> ```
> I feel this is more simpler.
please see 4/8


================
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?");
----------------
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?


================
Comment at: clang/lib/Sema/SemaModule.cpp:373
+      ModuleName = NamedMod->Name;
+      ModuleName += ":";
     }
----------------
ChuanqiXu wrote:
> We could move this below the if statement.
please see 4/8 and later


================
Comment at: clang/lib/Sema/SemaModule.cpp:477-480
+  } else if (getLangOpts().isCompilingModule()) {
+    Module *ThisModule = PP.getHeaderSearchInfo().lookupModule(
+        getLangOpts().CurrentModule, ExportLoc, false, false);
+    assert(ThisModule && "was expecting a module if building one");
----------------
ChuanqiXu wrote:
> What does it assert? I don't get the point.
that we should have created a module if the compilation task was to build one.  IIRC this was an existing assert .. but I will check.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118586