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