[PATCH] D113391: [Modules] Don't support clang module and c++20 module at the same time

Tue Nov 9 09:42:41 PST 2021

Bigcheese added inline comments.

================
Comment at: clang/test/ClangScanDeps/Inputs/header-search-pruning/cdb.json:3
+  "directory" : "DIR",
+  "command" : "clang -E DIR/header-search-pruning.cpp -Ibegin -I1 -Ia -I3 -I4 -I5 -I6 -Ib -I8 -Iend DEFINES -fmodules -fmodules-cache-path=DIR/module-cache -fimplicit-modules -fmodule-map-file=DIR/module.modulemap",
+  "file" : "DIR/header-search-pruning.cpp"
----------------
dblaikie wrote:
> aaron.ballman wrote:
> > jansvoboda11 wrote:
> > > ChuanqiXu wrote:
> > > > jansvoboda11 wrote:
> > > > > ChuanqiXu wrote:
> > > > > > jansvoboda11 wrote:
> > > > > > > Can you explain why `-fcxx-modules` is removed? We want to explicitly enable Clang modules for C++ inputs here. That's off by default in our downstream repo and we'd like to keep this upstream to prevent conflicts. (it's benign upstream)
> > > > > > According to the discussion in the link, I think it is in consensus that we decide to not support clang modules and c++20 modules at the same time. (I know many people may not have visited it. If any one disagree with the higher idea, I think it would be better to reply in that thread). 
> > > > > > So the original test case would fail.
> > > > > > 
> > > > > > > We want to explicitly enable Clang modules for C++ inputs here. 
> > > > > > 
> > > > > > I am not sure if I missed any thing. Personally, it looks like the test now would test clang modules for c++ inputs. Since it has `-fmodule` option so that clang module is enabled and the input is C++ (from its suffix). Do I misunderstand you?
> > > > > > According to the discussion in the link, I think it is in consensus that we decide to not support clang modules and c++20 modules at the same time. (I know many people may not have visited it. If any one disagree with the higher idea, I think it would be better to reply in that thread). 
> > > > > > So the original test case would fail.
> > > > > 
> > > > > What's the error message? As I say, we're enabling Clang modules for C++ input here, not C++20 modules.
> > > > > 
> > > > > > > We want to explicitly enable Clang modules for C++ inputs here. 
> > > > > > 
> > > > > > I am not sure if I missed any thing. Personally, it looks like the test now would test clang modules for c++ inputs. Since it has `-fmodule` option so that clang module is enabled and the input is C++ (from its suffix). Do I misunderstand you?
> > > > > 
> > > > > Right. What I'm saying is that in our downstream repo, `-fmodules` is not enough to enable Clang modules for C++ inputs, you need to do it via `-fcxx-modules`. And we'd like to keep it upstream, even though it's benign here, to avoid conflicts between the repos.
> > > > > What's the error message? As I say, we're enabling Clang modules for C++ input here, not C++20 modules.
> > > > 
> > > > The error message is added in this patch. After this patch landed, the original intentional behavior would be `-fmodules` to enable clang module and `-fcxx-modules` to enable C++20 modules.
> > > > 
> > > > > Right. What I'm saying is that in our downstream repo, -fmodules is not enough to enable Clang modules for C++ inputs, you need to do it via -fcxx-modules. And we'd like to keep it upstream, even though it's benign here, to avoid conflicts between the repos.
> > > > 
> > > > Got it. But it conflicts with the idea that disable clang module and c++20 module at the same time. Personally, I think it would be better to edit the code in downstream. @aaron.ballman sorry if I ping you too frequently. But I think now we need input from you.
> > > Ah, I understand now, thanks for explaining.
> > > 
> > > I'm worried about changing the behavior of a driver flag, we generally don't break existing driver options. Have you considered keeping the `-fmodules` and `-fcxx-modules` semantics intact and instead add new `-fno-c++20-modules` flag or something like that?
> > > I'm worried about changing the behavior of a driver flag, we generally don't break existing driver options. Have you considered keeping the -fmodules and -fcxx-modules semantics intact and instead add new -fno-c++20-modules flag or something like that?
> > 
> > The goal is not "let users disable C++20 modules", it's "ensure the user cannot enable two different kinds of modules at the same time". What I think we want to avoid is "C++20 mode, but with Clang modules instead of standard modules" or "C++20 mode, but with both clang and standard modules", etc. I think the support matrix that makes sense to me is:
> > 
> > C++20 mode: you get standard modules, no other module schemes are allowed
> > Pre-C++20 modes: you can opt into Clang modules or you can opt into C++20 modules
> > Non-C++ modes: you can opt into Clang modules (maybe we want to also support C++20 modules in C, but I'm less certain)
> > 
> > However, I'm not certain if other people agree. I'm basing this on the belief that we don't want modules features to have different semantics within the same program. While this does make it harder for people with a C++17 code base using Clang modules to upgrade to C++20, I suggest that users shouldn't enable -std=c++20 until their code base is ready to make that leap and that includes dealing with the incompatible modules features.
> > 
> > @dblaikie -- I've seen you responding to some of the WG21 discussions around modules. Do you have opinions here?
> Mixed feelings - it's still a pretty exploratory space.
> 
> Backwards compatibility's pretty important - though, yeah, seems some folks on the C++ committee have different ideas about that & insist C++20 presents a pretty hard break in terms of how libraries vend interfaces, etc. Sorry, that's a bit of an aside/only vaguely connected.
> 
> I'd /guess/ Google would want some way to combine C++20 and Clang Header Modules if/when we get to the rollout - and Apple's probably in that situation too?
> 
> I'm honestly probably a bit too out of the loop to make good calls here - I've weighed in on a few conversations to try to help move them forward here and there where I thought I could add value, but probably not informed enough really.
> 
> How far are Clang Header Modules from C++20 header units? Far enough off that we can't just treat things that are Clang Header Modules pre-C++20 as C++20 header units when building in C++20 mode?
> I'd /guess/ Google would want some way to combine C++20 and Clang Header Modules if/when we get to the rollout - and Apple's probably in that situation too?

Very much so. We would like Clang modules to just be modeled as C++20 Header Units, which was a large reason for Header Units existing in the first place.

Specifically module maps are how Clang implements the "set of implementation defined headers" requirement for C++20 Header Units.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113391