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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113391