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