[PATCH] D121588: [C++20][Modules][Driver][HU 1/N] Initial handling for -xc++-{system,user}-header.

[Iain Sandoe via Phabricator via cfe-commits]

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