iains marked 3 inline comments as done. iains added a comment. In D121588#3384799 <https://reviews.llvm.org/D121588#3384799>, @vsapsai wrote:
> Sorry, I'm pretty ignorant in this area but based on > >> The job construction is altered to build a C++20 header unit (rather than a >> PCH file, as would be the case for other headers). > > I want to clarify. The goal is to allow mixing PCH and PCM files, right? I > haven't double checked and maybe that have changed already but I think `-x > c++-header` started building .pcm instead of .pch with `-std=c++20`. That's > where my confusion about mixing PCH and PCM comes from. From my understanding of the modules group objectives: The (my at least understand of) objective is that when the user produces a C++20+ command line //**with no contradicting options**//, then the output would be standardised C++20 modules. As of this moment, clang modules and C++20 modules have some divergence (I understand that the goal is to unify - but that will take some time, I expect). If the user wants clang modules, then they should add "-fmodules" to the command line - which would switch off C++20 and produce a PCH header module as before 9perhaps built from several headers, where a C++20 HU only reflects one header), Perhaps we cannot make this objective yet - but I can say that the current choices in this code do not produce any regressions in the testsuite - so that is some indication that it could be possible, right now. There is a quite along discussion on this in https://reviews.llvm.org/D120540 ================ Comment at: clang/include/clang/Driver/Types.def:66 TYPE("c++-header", CXXHeader, PP_CXXHeader, "hh", phases::Preprocess, phases::Precompile) -TYPE("objective-c++-header-cpp-output", PP_ObjCXXHeader, INVALID, "mii", phases::Precompile) +TYPE("c++-header-unit-cpp-output", PP_CXXHeaderUnit,INVALID, "iih", phases::Precompile) +TYPE("c++-header-unit-header", CXXHUHeader, PP_CXXHeaderUnit,"hh", phases::Preprocess, phases::Precompile) ---------------- vsapsai wrote: > Sorry, it's not really related to your change but do you have a rule where > "ii" should go? It's just we have both "mii" and "iim" and I want to make > sure it should be "iih" and not "hii". I haven't tried to find a pattern here > myself, asking you first. > Sorry, it's not really related to your change but do you have a rule where > "ii" should go? It's just we have both "mii" and "iim" and I want to make > sure it should be "iih" and not "hii". I haven't tried to find a pattern here > myself, asking you first. My understanding is this: ObjectiveC/C++ append "I" and "ii" (mi and mii) C++ Modules-related code pre-pends "ii". so that a pre-processed header unit ==> "iih" and a pre-processed module ==> "iim". ================ Comment at: clang/lib/Frontend/FrontendOptions.cpp:30-31 .Case("cppm", Language::CXX) + .Case("iih", InputKind(Language::CXX).getPreprocessed()) .Case("iim", InputKind(Language::CXX).getPreprocessed()) .Case("cl", Language::OpenCL) ---------------- vsapsai wrote: > Given the other branches in this StringSwitch I would expect `.Cases("iih", > "iim", InputKind(Language::CXX).getPreprocessed())`. Is there a reason not to > do that (like differences between iih and iim) or is it accidental? accidental on someone's part, I expect .. but pre-existing (I have not [intentionally, at least] modified the behaviour of non-header unit module in this patch set) ================ Comment at: clang/test/Driver/cxx20-header-units-01.cpp:7 + +// RUN: %clang++ -### -std=c++20 -xc++-header-unit-header %S/Inputs/header-unit-01.hh 2>&1 | \ +// RUN: FileCheck -check-prefix=CHECK-ABS %s -DTDIR=%S/Inputs ---------------- vsapsai wrote: > What should happen in case of inconsistencies like `%clang++ -### -std=c++20 > -xc++-system-header %S/Inputs/header-unit-01.hh`? Or `-xc++-system-header > foo.h`? If the user states that `%S/Inputs/header-unit-01.hh` is a system header, I do not think that the driver is in a position to argue? ` -xc++-system-header foo.h ` again the user has made an explicit statement.. I suppose that we can always diagnose these things with warnings - but I do not think we can easily reject them (and we risk producing unhelpful output). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D121588/new/ https://reviews.llvm.org/D121588 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
