https://github.com/HighCommander4 requested changes to this pull request.
Thanks for the patch! The general approach here looks good. One issue that I see with the current implementation is that it makes the command-line flag `--header-insertion=never` have no effect. That's because, while the value of the command-line flag will be copied into the code completion options [here](https://searchfox.org/llvm/rev/107aa6a3d3ab96b7eec55e1ec5c3eabfa6ab2f9f/clang-tools-extra/clangd/tool/ClangdMain.cpp#943), that will then be overwritten in `codeComplete()` where we now do: ``` CodeCompleteOpts.InsertIncludes = Config::current().HeaderInsertion.Policy; ``` If the user didn't specify a `HeaderInsertion` config option, the value of `Config::current().HeaderInsertion.Policy` will be `IWYU` (the default specified in `Config.h`). and we overwrite the `NeverInsert` value with `IWYU`. This would break users who have `--header-insertion=never` specified today and expect it to continue to work. To get the command line flag to interact with the config option better, we need to handle it in [`FlagsConfigProvider`](https://searchfox.org/llvm/rev/107aa6a3d3ab96b7eec55e1ec5c3eabfa6ab2f9f/clang-tools-extra/clangd/tool/ClangdMain.cpp#655), which gives the command-line flag the ability to influence the value of `Config::current().HeaderInsertion.Policy`. We have a few options regarding the precedence, but I think the simplest to implement is to handle it similar to `--background-index` [here](https://searchfox.org/llvm/rev/107aa6a3d3ab96b7eec55e1ec5c3eabfa6ab2f9f/clang-tools-extra/clangd/tool/ClangdMain.cpp#711-713): if the flag's value is `NeverInsert`, then write that into the config object, otherwise leave it alone. The resulting behaviour will be: * If the user specifies `--header-insertion=never` on the command line, it takes precedence and disables header insertion globally, regardless of any config options. * If the user doesn't specify `--header-insertion=never` on the command line, the config file option is used if specified, falling backing to `IWYU` as the default. I think that should work well enough in practice -- let me know what you think. https://github.com/llvm/llvm-project/pull/128503 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
