aaron.ballman added inline comments.
================ Comment at: clang/include/clang/Basic/DiagnosticDriverKinds.td:162-163 "'%0': unable to use module files with this tool">; +def err_drv_clang_module_cxx_module_conflicts : Error < + "Unable to use clang module and c++20 module at the same time.">; def err_drv_clang_unsupported : Error< ---------------- I think we can get away with `err_drv_argument_not_allowed_with` for this diagnostic -- this also will tell the user *which* options are in conflict. ================ Comment at: clang/include/clang/Basic/DiagnosticFrontendKinds.td:244 DefaultFatal; +def err_conflicts_clang_module_and_cxx20_module : + Error<"Unable to use clang module and c++20 module at the same time.">; ---------------- I think it's reasonable to give a diagnostic from the driver when the user does this, but in the FE for a cc1 invocation, I'm a bit less sympathetic to giving people nice diagnostics. Do we need this diagnostic, or can we pick one option to be the "winner" in this case? ================ 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" ---------------- 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? 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
