Page MenuHomePhabricator

[clangd] Add support for HeaderInsertion in .clangd config file
Needs ReviewPublic

Authored by qchateau on Sep 21 2022, 1:27 PM.

Details

Reviewers
nridge
sammccall
Summary

Add ability to configure header insertion when accepting code
completion from the .clangd config file.
The command line option always take precedence.

Diff Detail

Event Timeline

qchateau created this revision.Sep 21 2022, 1:27 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 21 2022, 1:27 PM
qchateau requested review of this revision.Sep 21 2022, 1:27 PM

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.

clang-tools-extra/clangd/Config.h
27

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?

clang-tools-extra/clangd/ConfigFragment.h
292

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.

qchateau updated this revision to Diff 464492.Oct 1 2022, 5:28 AM
  • clangd: rename HeaderInsertion and IncludeInsertion, read value from Config