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