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

[Iain Sandoe via Phabricator via cfe-commits]

iains marked an inline comment as done.
iains added inline comments.


================
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();
----------------
ChuanqiXu wrote:
> 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?
> 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.

correct, in this case, the fact that Header Units are specific to the  C++20 implementation (they are quite different from clang header modules) allows us to tell the difference.

> BTW, may I ask the behavior is consistency with GCC?

yes, I discussed this with @urnathan (especially that it is very difficult to get consistent behaviour if we were to include-translate in the module purview).


================
Comment at: clang/lib/Lex/PPDirectives.cpp:2335
+                                         IsFirstIncludeOfFile)) {
+    // standard modules:
+    // If we are not in the GMF, then we textually include only
----------------
ChuanqiXu wrote:
> nit: It looks like we prefer to use `C++20 modules` over `standard modules`, although `standard modules` must be the right term.
since we are heading for C++23  ... perhaps now would be a good time to start using "standard modules"? (I can change to C++20 modules if there's objection).



================
Comment at: clang/lib/Parse/Parser.cpp:672
+    if (!getLangOpts().CPlusPlusModules || !Mod->isHeaderUnit() ||
+        getLangOpts().ModulesTS)
+      Actions.ActOnModuleInclude(Loc, Mod);
----------------
ChuanqiXu wrote:
> I think we could ignore `getLangOpts().ModulesTS` here.
well, we agreed that (for the present) we would try to avoid changing the behaviour w.r.t modules-ts (but spend no effort to complete the implementation) - and to remove it from comments; we can then take a pass through to remove the modules-ts behaviour (assuming that no-one is using it!)



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128981