[PATCH] D134384: [clangd] Add support for HeaderInsertion in .clangd config file

[Sam McCall via Phabricator via cfe-commits]

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