[PATCH] D134384: [clangd] Add support for HeaderInsertion in .clangd config file
Sam McCall via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Wed Sep 28 20:23:50 PDT 2022
sammccall added a comment.
Thanks, this is a great idea.
It also opens the door to suppressing includes of particular files later (e.g. by regex) with another config option, though we need to be careful of performance there.
================
Comment at: clang-tools-extra/clangd/Config.h:27
+#include "CodeComplete.h"
#include "support/Context.h"
----------------
Config shouldn't depend on feature headers as it gets included everywhere :-(
This is a pain point, in general we'd need to define the same enum in two places, but often we find a workaround.
Here I think CodeCompleteOpts.IncludeInsertion is only actually referenced in two places (one production + one test), so it seems like it might be simplest just to remove that member from CodeCompleteOpts and read it from Config directly instead?
================
Comment at: clang-tools-extra/clangd/ConfigFragment.h:292
+ /// - IWYU
+ llvm::Optional<Located<std::string>> HeaderInsertion;
};
----------------
I'm torn on the naming here.
I think `InsertIncludes` would be a much better name: Insert vs Insertion is simpler and more active, and Includes is more accurate and descriptive than Headers. On the other hand, using exactly the same naming as the command-line flag makes it clear that it's the same option.
I think *probably* we should do the rename - we'll eventually deprecate the command-line arg and it's better to have the good name in the long run.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D134384/new/
https://reviews.llvm.org/D134384
More information about the cfe-commits
mailing list