ChuanqiXu added inline comments.
================
Comment at: clang/include/clang/Lex/Preprocessor.h:434
+ /// Saw a 'module' identifier.
+ void handleModule(bool ATLTS) {
+ // This was the first module identifier and not preceded by any token
----------------
iains wrote:
> ChuanqiXu wrote:
> > What's the meaning of `ATLTS`?
> `AfterTopLevelTokenSeq` I guess it seemed a rather long name (but I do not
> mind to spell it out if that makes sense).
>
Oh, I feel better to spell it out.
================
Comment at: clang/lib/Lex/PPDirectives.cpp:2226-2227
+
+ // FIXME: We do not have a good way to disambiguate C++ clang modules from
+ // C++ standard modules (other than use/non-use of Header Units).
+ Module *SM = SuggestedModule.getModule();
----------------
iains wrote:
> ChuanqiXu wrote:
> > From what @rsmith said in https://reviews.llvm.org/D113391, it looks like
> > the ideal direction is to use C++ clang modules and C++ standard modules
> > together. So it looks like we couldn't disambiguate them from command line
> > options.
> Well, I think there is some more debate to have around how to solve this
> problem (i.e. it might be intended that clang++ modules and standard c++
> modules converge but as things stand we still need to support the cases that
> they have different behaviour, or break existing users)
> ... - but please let us not have that debate in this patch :-)
>
It is not good to break existing users. Generally, a breaking change patch
could be reverted directly... We must care about it to avoid unnecessary
efforts. And it looks like the current implementation wouldn't break any
existing users, right? Since it uses `isHeaderUnit()`. I remember `HeaderUnit`
is introduced by you so it shouldn't conflict with clang modules.
BTW, may I ask the behavior is consistency with GCC?
================
Comment at: clang/lib/Lex/PPDirectives.cpp:2335
+ IsFirstIncludeOfFile)) {
+ // standard modules:
+ // If we are not in the GMF, then we textually include only
----------------
nit: It looks like we prefer to use `C++20 modules` over `standard modules`,
although `standard modules` must be the right term.
================
Comment at: clang/lib/Parse/Parser.cpp:672
+ if (!getLangOpts().CPlusPlusModules || !Mod->isHeaderUnit() ||
+ getLangOpts().ModulesTS)
+ Actions.ActOnModuleInclude(Loc, Mod);
----------------
I think we could ignore `getLangOpts().ModulesTS` here.
================
Comment at: clang/test/Modules/cxx20-include-translation.cpp:10
+// RUN: %clang_cc1 -std=c++20 -xc++-user-header h4.h -emit-header-unit -o
h4.pcm
+
+// RUN: %clang_cc1 -std=c++20 Xlate.cpp -emit-module-interface -o Xlate.pcm \
----------------
iains wrote:
> ChuanqiXu wrote:
> > Maybe we need an example `h5.h` which is not pre-compiled as a header unit
> > and it would be handled correctly.
> I am not sure if you mean that the un-converted header would be included in
> the GMF or in the module purview - IMO we already have test-cases that cover
> this, but I do not mind adding something extra if there's a missing case?
>
> I am not sure if you mean that the un-converted header would be included in
> the GMF or in the module purview
At least in the GMF.
> IMO we already have test-cases that cover this, but I do not mind adding
> something extra if there's a missing case?
Yeah, I feel better to have more coverage.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D128981/new/
https://reviews.llvm.org/D128981
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
