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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
