jansvoboda11 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" ---------------- 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? 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
