ChuanqiXu accepted this revision. ChuanqiXu added a comment. This revision is now accepted and ready to land.
Then LGTM if all the comments addressed. ================ 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: > > 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). Got it. Thanks. ================ Comment at: clang/lib/Lex/PPDirectives.cpp:2335 + IsFirstIncludeOfFile)) { + // standard modules: + // If we are not in the GMF, then we textually include only ---------------- iains wrote: > 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). > It is OK to me. But I feel better if we use `C++ standard modules` since modules is a feature lives everywhere. ================ Comment at: clang/lib/Parse/Parser.cpp:672 + if (!getLangOpts().CPlusPlusModules || !Mod->isHeaderUnit() || + getLangOpts().ModulesTS) + Actions.ActOnModuleInclude(Loc, Mod); ---------------- iains wrote: > 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!) > Given the previous status of C++ modules, it is hard to believe there is existing users for it. So I think it should be fine to remove the support for module-ts. 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
