[PATCH] D128981: [C++20][Modules] Implement include translation.

[Chuanqi Xu via Phabricator via cfe-commits]

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