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