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

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu abandoned this revision.
ChuanqiXu added a comment.

Oh, OK. If this is not the direction the community want, we might not go on 
this direction.


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

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

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith requested changes to this revision.
rsmith added a comment.
This revision now requires changes to proceed.

The ideal situation here would be to have a single semantic model that is a 
superset of the various combinations of behaviors we want. Further entrenching 
Clang modules and C++20 modules as being different things would be a backwards 
step, and would be harmful for users of Clang modules who want to incrementally 
adopt C++20 modules. We shouldn't have a separate language option flag for 
"clang modules" -- that should be a different kind of module, not a different 
compilation mode; if a clang header module needs different treatment from a 
C++20 header unit, we should base that on the kind of the current 
`clang::Module`, not on a language option.

In terms of what the various `LangOptions` flags mean:

`LangOptions::Modules` corresponds to the expectation that modular headers will 
be imported rather than entered textually.
`LangOptions::ModulesTS` enables C++ Modules TS language syntax. This 
corresponds to `-fmodules-ts`. We should probably deprecate it.
`LangOptions::CPlusPlusModules` enables C++20 modules syntax. This is enabled 
by `-std=c++20`.
`LangOptions::ModulesLocalVisibility` indicates that in this compilation, 
visibility is not monotonically increasing. This corresponds to 
`-fmodules-local-submodule-visibility`. This is the case when we might compile 
multiple source files or header units into a single AST, and we want to reset 
the visibility state at the end of each  such unit. For example, this happens 
when building a Clang module containing multiple headers. This really should be 
enabled automatically when appropriate, but for now it's enabled by default 
with `CPlusPlusModules` and `ModulesTS`.

The only combination that's incompatible here is `ModulesTS` plus 
`CPlusPlusModules`, because they have overlapping but different syntax.


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

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

[Michael Spencer via Phabricator via cfe-commits]

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 

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

[David Blaikie via Phabricator via cfe-commits]

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 

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

[Aaron Ballman via Phabricator via cfe-commits]

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 

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

[Jan Svoboda via Phabricator via cfe-commits]

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

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

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu updated this revision to Diff 385765.
ChuanqiXu added a reviewer: jansvoboda11.
ChuanqiXu added a comment.

Undo unnecessary style change in *.json files


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113391

Files:
  clang-tools-extra/clang-tidy/ClangTidy.cpp
  clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.cpp
  clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-include.cpp
  clang-tools-extra/test/pp-trace/pp-trace-modules.cpp
  clang/include/clang/Basic/DiagnosticDriverKinds.td
  clang/include/clang/Basic/DiagnosticFrontendKinds.td
  clang/include/clang/Basic/Features.def
  clang/include/clang/Basic/LangOptions.def
  clang/include/clang/Basic/LangOptions.h
  clang/include/clang/Driver/Options.td
  clang/include/clang/Lex/Preprocessor.h
  clang/lib/ARCMigrate/ObjCMT.cpp
  clang/lib/AST/Decl.cpp
  clang/lib/AST/DeclCXX.cpp
  clang/lib/AST/DeclObjC.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/ASTUnit.cpp
  clang/lib/Frontend/CompilerInstance.cpp
  clang/lib/Frontend/FrontendAction.cpp
  clang/lib/Frontend/FrontendActions.cpp
  clang/lib/Lex/PPDirectives.cpp
  clang/lib/Lex/Preprocessor.cpp
  clang/lib/Parse/ParseObjc.cpp
  clang/lib/Sema/SemaCodeComplete.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclObjC.cpp
  clang/lib/Sema/SemaLookup.cpp
  clang/lib/Sema/SemaTemplate.cpp
  clang/lib/Sema/SemaType.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTReaderDecl.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/test/ClangScanDeps/Inputs/header-search-pruning/cdb.json
  clang/test/ClangScanDeps/Inputs/module_fmodule_name_cdb.json
  clang/test/ClangScanDeps/Inputs/modules_cdb.json
  clang/test/ClangScanDeps/Inputs/modules_cdb_by_mod_name.json
  clang/test/ClangScanDeps/Inputs/modules_cdb_clangcl.json
  clang/test/ClangScanDeps/Inputs/modules_cdb_clangcl_by_mod_name.json
  clang/test/ClangScanDeps/Inputs/modules_inferred_cdb.json
  clang/test/Driver/conflict-clang-module-and-cxx20-module.cpp
  clang/test/Driver/modules.mm
  clang/test/Modules/cxx20.cpp
  clang/test/Modules/ms-enums.cpp
  clang/test/SemaCXX/compare-modules-cxx2a.cpp
  clang/tools/libclang/Indexing.cpp

Index: clang/tools/libclang/Indexing.cpp
===
--- clang/tools/libclang/Indexing.cpp
+++ clang/tools/libclang/Indexing.cpp
@@ -604,7 +604,7 @@
 PPOpts.DetailedRecord = true;
   }
 
-  if (!requestedToGetTU && !CInvok->getLangOpts()->Modules)
+  if (!requestedToGetTU && !CInvok->getLangOpts()->hasModules())
 PPOpts.DetailedRecord = false;
 
   // Unless the user specified that they want the preamble on the first parse
Index: clang/test/SemaCXX/compare-modules-cxx2a.cpp
===
--- clang/test/SemaCXX/compare-modules-cxx2a.cpp
+++ clang/test/SemaCXX/compare-modules-cxx2a.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -triple x86_64-apple-darwin -fcxx-exceptions -verify -std=c++2a -fmodules -I%S/Inputs %s -fno-modules-error-recovery
+// RUN: %clang_cc1 -triple x86_64-apple-darwin -fcxx-exceptions -verify -std=c++2a -fmodules -fno-cxx-modules -I%S/Inputs %s -fno-modules-error-recovery
 
 #pragma clang module build compare
 module compare {
Index: clang/test/Modules/ms-enums.cpp
===
--- clang/test/Modules/ms-enums.cpp
+++ clang/test/Modules/ms-enums.cpp
@@ -1,5 +1,5 @@
 // RUN: rm -rf %t
-// RUN: %clang_cc1 -triple x86_64-windows-msvc -fms-extensions -fms-compatibility -x c++ -std=c++20 -fmodules-cache-path=%t -fmodules -fimplicit-module-maps -I %S/Inputs/ms-enums %s -verify -fno-modules-error-recovery
+// RUN: %clang_cc1 -triple x86_64-windows-msvc -fms-extensions -fms-compatibility -x c++ -fmodules-cache-path=%t -fmodules -fimplicit-module-maps -I %S/Inputs/ms-enums %s -verify -fno-modules-error-recovery
 
 #include "B.h"
 // expected-note@A.h:1 {{declaration here is not visible}}
Index: clang/test/Modules/cxx20.cpp
===
--- clang/test/Modules/cxx20.cpp
+++ clang/test/Modules/cxx20.cpp
@@ -1,5 +1,5 @@
 // RUN: rm -rf %t
-// RUN: %clang_cc1 -x c++ -std=c++20 -fmodules-cache-path=%t -fmodules -fimplicit-module-maps -I %S/Inputs/cxx20 %s -verify -fno-modules-error-recovery
+// RUN: %clang_cc1 -x c++ -std=c++20 -fmodules-cache-path=%t -fmodules -fno-cxx-modules -fimplicit-module-maps -I %S/Inputs/cxx20 %s -verify -fno-modules-error-recovery
 
 // expected-no-diagnostics
 
Index: clang/test/Driver/modules.mm
===
--- clang/test/Driver/modules.mm
+++ clang/test/Driver/modules.mm
@@ -1,11 +1,11 @@
 // RUN: %clang -### %s 2>&1 | FileCheck -check-prefix=CHECK-NO-MODULES %s
-// RUN: %clang -fcxx-modules -### %s 2>&1 | FileCheck 

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

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added inline comments.



Comment at: clang/test/ClangScanDeps/Inputs/header-search-pruning/cdb.json:1
-[
-  {
-"directory": "DIR",
-"command": "clang -E DIR/header-search-pruning.cpp -Ibegin -I1 -Ia -I3 -I4 
-I5 -I6 -Ib -I8 -Iend DEFINES -fmodules -fcxx-modules 
-fmodules-cache-path=DIR/module-cache -fimplicit-modules 
-fmodule-map-file=DIR/module.modulemap",
-"file": "DIR/header-search-pruning.cpp"
-  }
-]
+[{
+  "directory" : "DIR",

MyDeveloperDay wrote:
> jansvoboda11 wrote:
> > ChuanqiXu wrote:
> > > jansvoboda11 wrote:
> > > > Please undo the whitespace changes in `ClangScanDeps` tests.
> > > It looks like that it's formatted by clang-format surprisingly. I would 
> > > undo this manually.
> > Thanks.
> Just as a drive by assuming you are using a fairly recent clang-format, then 
> clang-format should ONLY clang-format .json files if you have added the 
> following code into your .clang-format 
> 
> ```
> BasedOnStyle: LLVM
> ColumnLimit: 0
> AlwaysBreakTemplateDeclarations: No
> Language: Cpp
> ---
> Language: Json
> BasedOnStyle: LLVM
> ```
> 
> Doing so would render like this
> 
> ```
> $ clang-format cdb.json
> [
>   {
> "directory": "DIR",
> "command": "clang -E DIR/header-search-pruning.cpp -Ibegin -I1 -Ia -I3 
> -I4 -
> I5 -I6 -Ib -I8 -Iend DEFINES -fmodules -fcxx-modules 
> -fmodules-cache-path=DIR/mo
> dule-cache -fimplicit-modules -fmodule-map-file=DIR/module.modulemap",
> "file": "DIR/header-search-pruning.cpp"
>   }
> ]
> ```
Oh, thanks for your kindly guide. But I have updated this manually hhh. BTW, 
the clang-format I am using is:
```
git diff -U0 --no-color --relative HEAD^ | 
clang/tools/clang-format/clang-format-diff.py -p1 -i
```

This one is recommended by the authority documentation so I think many others 
would use this too.



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:
> > > 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 

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

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added inline comments.



Comment at: clang/test/ClangScanDeps/Inputs/header-search-pruning/cdb.json:1
-[
-  {
-"directory": "DIR",
-"command": "clang -E DIR/header-search-pruning.cpp -Ibegin -I1 -Ia -I3 -I4 
-I5 -I6 -Ib -I8 -Iend DEFINES -fmodules -fcxx-modules 
-fmodules-cache-path=DIR/module-cache -fimplicit-modules 
-fmodule-map-file=DIR/module.modulemap",
-"file": "DIR/header-search-pruning.cpp"
-  }
-]
+[{
+  "directory" : "DIR",

jansvoboda11 wrote:
> ChuanqiXu wrote:
> > jansvoboda11 wrote:
> > > Please undo the whitespace changes in `ClangScanDeps` tests.
> > It looks like that it's formatted by clang-format surprisingly. I would 
> > undo this manually.
> Thanks.
Just as a drive by assuming you are using a fairly recent clang-format, then 
clang-format should ONLY clang-format .json files if you have added the 
following code into your .clang-format 

```
BasedOnStyle: LLVM
ColumnLimit: 0
AlwaysBreakTemplateDeclarations: No
Language: Cpp
---
Language: Json
BasedOnStyle: LLVM
```

Doing so would render like this

```
$ clang-format cdb.json
[
  {
"directory": "DIR",
"command": "clang -E DIR/header-search-pruning.cpp -Ibegin -I1 -Ia -I3 -I4 -
I5 -I6 -Ib -I8 -Iend DEFINES -fmodules -fcxx-modules -fmodules-cache-path=DIR/mo
dule-cache -fimplicit-modules -fmodule-map-file=DIR/module.modulemap",
"file": "DIR/header-search-pruning.cpp"
  }
]
```


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

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

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 added inline comments.



Comment at: clang/test/ClangScanDeps/Inputs/header-search-pruning/cdb.json:1
-[
-  {
-"directory": "DIR",
-"command": "clang -E DIR/header-search-pruning.cpp -Ibegin -I1 -Ia -I3 -I4 
-I5 -I6 -Ib -I8 -Iend DEFINES -fmodules -fcxx-modules 
-fmodules-cache-path=DIR/module-cache -fimplicit-modules 
-fmodule-map-file=DIR/module.modulemap",
-"file": "DIR/header-search-pruning.cpp"
-  }
-]
+[{
+  "directory" : "DIR",

ChuanqiXu wrote:
> jansvoboda11 wrote:
> > Please undo the whitespace changes in `ClangScanDeps` tests.
> It looks like that it's formatted by clang-format surprisingly. I would undo 
> this manually.
Thanks.



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:
> > 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.


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

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

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added inline comments.



Comment at: clang/test/ClangScanDeps/Inputs/header-search-pruning/cdb.json:1
-[
-  {
-"directory": "DIR",
-"command": "clang -E DIR/header-search-pruning.cpp -Ibegin -I1 -Ia -I3 -I4 
-I5 -I6 -Ib -I8 -Iend DEFINES -fmodules -fcxx-modules 
-fmodules-cache-path=DIR/module-cache -fimplicit-modules 
-fmodule-map-file=DIR/module.modulemap",
-"file": "DIR/header-search-pruning.cpp"
-  }
-]
+[{
+  "directory" : "DIR",

jansvoboda11 wrote:
> Please undo the whitespace changes in `ClangScanDeps` tests.
It looks like that it's formatted by clang-format surprisingly. I would undo 
this manually.



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:
> 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?


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

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

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 added inline comments.



Comment at: clang/test/ClangScanDeps/Inputs/header-search-pruning/cdb.json:1
-[
-  {
-"directory": "DIR",
-"command": "clang -E DIR/header-search-pruning.cpp -Ibegin -I1 -Ia -I3 -I4 
-I5 -I6 -Ib -I8 -Iend DEFINES -fmodules -fcxx-modules 
-fmodules-cache-path=DIR/module-cache -fimplicit-modules 
-fmodule-map-file=DIR/module.modulemap",
-"file": "DIR/header-search-pruning.cpp"
-  }
-]
+[{
+  "directory" : "DIR",

Please undo the whitespace changes in `ClangScanDeps` tests.



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"

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)


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

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

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added inline comments.



Comment at: clang/lib/Driver/ToolChains/Clang.cpp:3445-3447
-  // -fmodules enables the use of precompiled modules (off by default).
-  // Users can pass -fno-cxx-modules to turn off modules support for
-  // C++/Objective-C++ programs.

From our discussion, `fmodules` shouldn't be suppressed by `fno-cxx-modules`. 
And the tests shows that is wouldn't be a problem to relax this.


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

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

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

In D113391#3115410 , @tschuett wrote:

> It would enforce that there is exactly one module mode live.

Yeah. But my consideration is that it would enforce another variable and it 
couldn't eliminate the two existing variables. So it looks redundant to me. We 
have made this check in driver and frontend part. So it looks sufficient to me.
I admit that the current method has limitations. For example, it is possible 
that both `ClangModules` and `CPlusPlusModules` could be wrote as true  in 
Lexer part and Sema part (Maybe we could add an assertion in `hasModules()`). 
And your suggestion could solve this. But I have another question about the 
implementation, where should we put this variable and when should we fill the 
value for it? I failed to use a new variable in LangOpts and initialize it in 
constructor of LangOpts since both `ClangModules` and `CPlusPlusModules` is 
zero.
And even in this suggestion, user could still access `ClangModules` and 
`CPlusPlusModules`. And they could rewrite them too. So I am wondering if it 
would be more confusing.


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

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

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu updated this revision to Diff 385680.
ChuanqiXu added a comment.
Herald added subscribers: carlosgalvezp, kbarton, nemanjai.
Herald added a project: clang-tools-extra.

Update part in clang-tools-extra


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113391

Files:
  clang-tools-extra/clang-tidy/ClangTidy.cpp
  clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.cpp
  clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-include.cpp
  clang-tools-extra/test/pp-trace/pp-trace-modules.cpp
  clang/include/clang/Basic/DiagnosticDriverKinds.td
  clang/include/clang/Basic/DiagnosticFrontendKinds.td
  clang/include/clang/Basic/Features.def
  clang/include/clang/Basic/LangOptions.def
  clang/include/clang/Basic/LangOptions.h
  clang/include/clang/Driver/Options.td
  clang/include/clang/Lex/Preprocessor.h
  clang/lib/ARCMigrate/ObjCMT.cpp
  clang/lib/AST/Decl.cpp
  clang/lib/AST/DeclCXX.cpp
  clang/lib/AST/DeclObjC.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/ASTUnit.cpp
  clang/lib/Frontend/CompilerInstance.cpp
  clang/lib/Frontend/FrontendAction.cpp
  clang/lib/Frontend/FrontendActions.cpp
  clang/lib/Lex/PPDirectives.cpp
  clang/lib/Lex/Preprocessor.cpp
  clang/lib/Parse/ParseObjc.cpp
  clang/lib/Sema/SemaCodeComplete.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclObjC.cpp
  clang/lib/Sema/SemaLookup.cpp
  clang/lib/Sema/SemaTemplate.cpp
  clang/lib/Sema/SemaType.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTReaderDecl.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/test/ClangScanDeps/Inputs/header-search-pruning/cdb.json
  clang/test/ClangScanDeps/Inputs/module_fmodule_name_cdb.json
  clang/test/ClangScanDeps/Inputs/modules_cdb.json
  clang/test/ClangScanDeps/Inputs/modules_cdb_by_mod_name.json
  clang/test/ClangScanDeps/Inputs/modules_cdb_clangcl.json
  clang/test/ClangScanDeps/Inputs/modules_cdb_clangcl_by_mod_name.json
  clang/test/ClangScanDeps/Inputs/modules_inferred_cdb.json
  clang/test/Driver/conflict-clang-module-and-cxx20-module.cpp
  clang/test/Driver/modules.mm
  clang/test/Modules/cxx20.cpp
  clang/test/Modules/ms-enums.cpp
  clang/test/SemaCXX/compare-modules-cxx2a.cpp
  clang/tools/libclang/Indexing.cpp

Index: clang/tools/libclang/Indexing.cpp
===
--- clang/tools/libclang/Indexing.cpp
+++ clang/tools/libclang/Indexing.cpp
@@ -604,7 +604,7 @@
 PPOpts.DetailedRecord = true;
   }
 
-  if (!requestedToGetTU && !CInvok->getLangOpts()->Modules)
+  if (!requestedToGetTU && !CInvok->getLangOpts()->hasModules())
 PPOpts.DetailedRecord = false;
 
   // Unless the user specified that they want the preamble on the first parse
Index: clang/test/SemaCXX/compare-modules-cxx2a.cpp
===
--- clang/test/SemaCXX/compare-modules-cxx2a.cpp
+++ clang/test/SemaCXX/compare-modules-cxx2a.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -triple x86_64-apple-darwin -fcxx-exceptions -verify -std=c++2a -fmodules -I%S/Inputs %s -fno-modules-error-recovery
+// RUN: %clang_cc1 -triple x86_64-apple-darwin -fcxx-exceptions -verify -std=c++2a -fmodules -fno-cxx-modules -I%S/Inputs %s -fno-modules-error-recovery
 
 #pragma clang module build compare
 module compare {
Index: clang/test/Modules/ms-enums.cpp
===
--- clang/test/Modules/ms-enums.cpp
+++ clang/test/Modules/ms-enums.cpp
@@ -1,5 +1,5 @@
 // RUN: rm -rf %t
-// RUN: %clang_cc1 -triple x86_64-windows-msvc -fms-extensions -fms-compatibility -x c++ -std=c++20 -fmodules-cache-path=%t -fmodules -fimplicit-module-maps -I %S/Inputs/ms-enums %s -verify -fno-modules-error-recovery
+// RUN: %clang_cc1 -triple x86_64-windows-msvc -fms-extensions -fms-compatibility -x c++ -fmodules-cache-path=%t -fmodules -fimplicit-module-maps -I %S/Inputs/ms-enums %s -verify -fno-modules-error-recovery
 
 #include "B.h"
 // expected-note@A.h:1 {{declaration here is not visible}}
Index: clang/test/Modules/cxx20.cpp
===
--- clang/test/Modules/cxx20.cpp
+++ clang/test/Modules/cxx20.cpp
@@ -1,5 +1,5 @@
 // RUN: rm -rf %t
-// RUN: %clang_cc1 -x c++ -std=c++20 -fmodules-cache-path=%t -fmodules -fimplicit-module-maps -I %S/Inputs/cxx20 %s -verify -fno-modules-error-recovery
+// RUN: %clang_cc1 -x c++ -std=c++20 -fmodules-cache-path=%t -fmodules -fno-cxx-modules -fimplicit-module-maps -I %S/Inputs/cxx20 %s -verify -fno-modules-error-recovery
 
 // expected-no-diagnostics
 
Index: clang/test/Driver/modules.mm
===
--- clang/test/Driver/modules.mm
+++ clang/test/Driver/modules.mm
@@ -1,11 +1,11 @@
 // RUN: %clang -### %s 2>&1 | FileCheck -check-prefix=CHECK-NO-MODULES %s
-// RUN: %clang 

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

[Thorsten via Phabricator via cfe-commits]

tschuett added a comment.

It would enforce that there is exactly module mode live.


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

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

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

In D113391#3115080 , @tschuett wrote:

> This sounds like an enum.
>
>   enum class ModuleKind {
>   C++20,
>   Clang,
>   Unsupported
>   }

In fact, there is another value `ModuleTS`. But it looks not bad. And the 
variable `CPlusPlusModules` and `ClangModules` comes from options directly (See 
Options.td for details). So it looks like we couldn't eliminate 
`CPlusPlusModules` and `ClangModules` after we introduce this enum. So the 
overall complexity didn't get decreased to me.


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

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

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu updated this revision to Diff 385440.
ChuanqiXu added reviewers: aaron.ballman, rsmith, urnathan, dblaikie.
ChuanqiXu set the repository for this revision to rG LLVM Github Monorepo.
ChuanqiXu added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113391

Files:
  clang/include/clang/Basic/DiagnosticDriverKinds.td
  clang/include/clang/Basic/DiagnosticFrontendKinds.td
  clang/include/clang/Basic/Features.def
  clang/include/clang/Basic/LangOptions.def
  clang/include/clang/Basic/LangOptions.h
  clang/include/clang/Driver/Options.td
  clang/include/clang/Lex/Preprocessor.h
  clang/lib/ARCMigrate/ObjCMT.cpp
  clang/lib/AST/Decl.cpp
  clang/lib/AST/DeclCXX.cpp
  clang/lib/AST/DeclObjC.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/ASTUnit.cpp
  clang/lib/Frontend/CompilerInstance.cpp
  clang/lib/Frontend/FrontendAction.cpp
  clang/lib/Frontend/FrontendActions.cpp
  clang/lib/Lex/PPDirectives.cpp
  clang/lib/Lex/Preprocessor.cpp
  clang/lib/Parse/ParseObjc.cpp
  clang/lib/Sema/SemaCodeComplete.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclObjC.cpp
  clang/lib/Sema/SemaLookup.cpp
  clang/lib/Sema/SemaTemplate.cpp
  clang/lib/Sema/SemaType.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTReaderDecl.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/test/ClangScanDeps/Inputs/header-search-pruning/cdb.json
  clang/test/ClangScanDeps/Inputs/module_fmodule_name_cdb.json
  clang/test/ClangScanDeps/Inputs/modules_cdb.json
  clang/test/ClangScanDeps/Inputs/modules_cdb_by_mod_name.json
  clang/test/ClangScanDeps/Inputs/modules_cdb_clangcl.json
  clang/test/ClangScanDeps/Inputs/modules_cdb_clangcl_by_mod_name.json
  clang/test/ClangScanDeps/Inputs/modules_inferred_cdb.json
  clang/test/Driver/conflict-clang-module-and-cxx20-module.cpp
  clang/test/Modules/cxx20.cpp
  clang/test/Modules/ms-enums.cpp
  clang/test/SemaCXX/compare-modules-cxx2a.cpp
  clang/tools/libclang/Indexing.cpp

Index: clang/tools/libclang/Indexing.cpp
===
--- clang/tools/libclang/Indexing.cpp
+++ clang/tools/libclang/Indexing.cpp
@@ -604,7 +604,7 @@
 PPOpts.DetailedRecord = true;
   }
 
-  if (!requestedToGetTU && !CInvok->getLangOpts()->Modules)
+  if (!requestedToGetTU && !CInvok->getLangOpts()->hasModules())
 PPOpts.DetailedRecord = false;
 
   // Unless the user specified that they want the preamble on the first parse
Index: clang/test/SemaCXX/compare-modules-cxx2a.cpp
===
--- clang/test/SemaCXX/compare-modules-cxx2a.cpp
+++ clang/test/SemaCXX/compare-modules-cxx2a.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -triple x86_64-apple-darwin -fcxx-exceptions -verify -std=c++2a -fmodules -I%S/Inputs %s -fno-modules-error-recovery
+// RUN: %clang_cc1 -triple x86_64-apple-darwin -fcxx-exceptions -verify -std=c++2a -fmodules -fno-cxx-modules -I%S/Inputs %s -fno-modules-error-recovery
 
 #pragma clang module build compare
 module compare {
Index: clang/test/Modules/ms-enums.cpp
===
--- clang/test/Modules/ms-enums.cpp
+++ clang/test/Modules/ms-enums.cpp
@@ -1,5 +1,5 @@
 // RUN: rm -rf %t
-// RUN: %clang_cc1 -triple x86_64-windows-msvc -fms-extensions -fms-compatibility -x c++ -std=c++20 -fmodules-cache-path=%t -fmodules -fimplicit-module-maps -I %S/Inputs/ms-enums %s -verify -fno-modules-error-recovery
+// RUN: %clang_cc1 -triple x86_64-windows-msvc -fms-extensions -fms-compatibility -x c++ -fmodules-cache-path=%t -fmodules -fimplicit-module-maps -I %S/Inputs/ms-enums %s -verify -fno-modules-error-recovery
 
 #include "B.h"
 // expected-note@A.h:1 {{declaration here is not visible}}
Index: clang/test/Modules/cxx20.cpp
===
--- clang/test/Modules/cxx20.cpp
+++ clang/test/Modules/cxx20.cpp
@@ -1,5 +1,5 @@
 // RUN: rm -rf %t
-// RUN: %clang_cc1 -x c++ -std=c++20 -fmodules-cache-path=%t -fmodules -fimplicit-module-maps -I %S/Inputs/cxx20 %s -verify -fno-modules-error-recovery
+// RUN: %clang_cc1 -x c++ -std=c++20 -fmodules-cache-path=%t -fmodules -fno-cxx-modules -fimplicit-module-maps -I %S/Inputs/cxx20 %s -verify -fno-modules-error-recovery
 
 // expected-no-diagnostics
 
Index: clang/test/Driver/conflict-clang-module-and-cxx20-module.cpp
===
--- /dev/null
+++ clang/test/Driver/conflict-clang-module-and-cxx20-module.cpp
@@ -0,0 +1,7 @@
+// RUN: %clangxx -### -c %s -std=c++20 -fmodules 2>&1 | FileCheck %s
+// RUN: %clangxx -### -c %s -std=c++2a -fmodules 2>&1 | FileCheck %s
+// RUN: %clangxx -### -c %s -std=c++20 -fmodules -fno-cxx-modules 2>&1 | FileCheck %s --check-prefix=FINE
+// RUN: %clangxx -###