[PATCH] D120540: [Driver] Enable to use C++20 modules standalone by -fcxx-modules

[Iain Sandoe via Phabricator via cfe-commits]

iains added a comment.

In D120540#3359446 <https://reviews.llvm.org/D120540#3359446>, @ChuanqiXu wrote:

> In D120540#3359394 <https://reviews.llvm.org/D120540#3359394>, @iains wrote:
>
>> 1. I agree 100% that the driver needs to be able to process in "c++20 modules mode";
>
> Yeah, not all the projects could be able to run in c++20 mode. We use `-std=c++17` + `-fcoroutines-ts` for C++20 coroutine before.
>
>> there is some handling of sources that should be changed accordingly.
>
> If I don't misunderstand, I guess we don't. Since all the code about C++20 modules are controlled by `CPlusPlusModules` variable. I think there wouldn't be codes controlled by `Cpp20` variables.

Actually, the comment was intending to refer to the driver specifically - since (for example) we have to disambiguate PCH jobs from C++20 Header Unit jobs - in the Front End, as of now we have been using CPlusPlus modules as the indicator for C++20 modules.  There is still scope for confusion if that is set at the same time as alternates...

>> 2. I believe that it is a general objective of the tooling folks (roughly SG15 participants) that C++20 modules should (eventually) be considered automatic for C++20+
>
> If I understand correctly, clang would enable C++20 modules by default if C++20 is enabled.

Yes, exactly, as is the case right now.

>> 3. There is at least one request from tooling folks that there should be an option to disable modules (even when in C++20  mode); this is a practical measure to avoid the case that it is impossible to build a TU because of some potential modules-related bug ...
>
> I think `-fno-cxxmodules` could be the option now.

so long as this is decoupled from any clang modules meanings?

>> 4. IMO it becomes increasingly important to try and decouple the clang modules from C++20 modules as much as possible.
>
> 100% agree. I think it should be beneficial to decouple them from command line, implementation and comments. (Now many comments are not precise. For example, when we talk about a `Module`. we are referring a module unit indeed. There are other examples.)

It would be friendly to the user to reject command line options that are not appropriate to the "current modules mode" - since there are ≈ 60+ modules-related command line options it is already very easy to be confused.  To do this, the driver needs to establish the "current modules mode" early (even before it builds jobs, since as noted above some jobs build differently depending on the assumed mode).

>> So .. I was going to suggest that we might introduce -fmodules= {none, clang, c++20, ...} 
>> with defaults picked:
>>
>>    fmodules => clang (i.e. the current meaning)
>>   
>>   !fmodules && C++20+ => c++20 (i.e. the objective of (2) above
>>   Where there are other flags that imply C++20 that can switch c++20 mode as well
>>   
>>   otherwise the default would be "none"
>>
>> .. this provides for (3) ... since -std=c++20 -fmodules=none could be used.
>>
>> I do not have a patch for this proposal as of this time (my current patches assume (2) but do not meet the objective of (3))
>
> The proposal is good to me though. I sent a patch (https://reviews.llvm.org/D113391) before to forbid the mixed use of clang modules and c++20 modules since it might be confusing if we use the combination `-fmodules -std=c++20`. But the comment shows that the current users of Clang Modules (mainly Google and Apple) wish a smooth switch from clang module to c++20 module. So I think the proposal which would break the current use cases would be not easy to land. 
> (This patch wouldn't break any use case I think).

Well, I think neither proposal breaks current use-cases - the selection of defaults is designed to make current command lines do exactly the same as they do now.

My comments are not intended as a blocker for your patch -  but simply to offer a suggestion for a more generic handling of the same objective.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120540