dblaikie 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" ---------------- 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? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D113391/new/ https://reviews.llvm.org/D113391 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
