This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Add option to always insert headers with <> instead of ""
Needs ReviewPublic

Authored by ADKaster on Mar 11 2023, 3:46 AM.

Details

Reviewers
nridge
Summary

Projects can now add the following config fragment to their .clangd:

Style:
  IncludeDelimiter: AlwaysBrackets

to force headers inserted via the --header-insertion=iwyu mode to have
<> around them in all cases, rather than relying on whether the header
was included via -isystem or not.

Ref https://github.com/clangd/clangd/issues/1247

This solution does not allow forcing "" in all cases, nor does it affect
other clang tools like clang-format.

Diff Detail

Event Timeline

ADKaster created this revision.Mar 11 2023, 3:46 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 11 2023, 3:46 AM
ADKaster requested review of this revision.Mar 11 2023, 3:46 AM

If this patch is accepted, I don't have commit access. If someone could commit as "Andrew Kaster <akaster@serenityos.org>" that would be much appreciated.

nridge added a subscriber: nridge.Mar 11 2023, 6:44 PM

Thanks for the patch!

May I suggest the flag name IncludeDelimiter, with values Auto and AlwaysBrackets (with the default being Auto)? This leaves room for other styles in the future if desired.

It would mean a slight implementation change since the YAML representation would now be a string rather than a bool, but nothing too involved; you can use the existing code for parsing IndexBlock::Background (whose allowed values are Build and Skip) as a guide.

ADKaster updated this revision to Diff 506409.Mar 19 2023, 11:05 AM

Change from a boolean parameter to an enum.

ADKaster edited the summary of this revision. (Show Details)Mar 19 2023, 11:14 AM
ADKaster added a reviewer: nridge.
ADKaster edited the summary of this revision. (Show Details)
ADKaster added inline comments.Mar 19 2023, 11:20 AM
clang-tools-extra/clangd/Headers.cpp
353

I couldn't tell looking at other code in clangd whether it would be preferred to fetch the value from Config::current() here every time, or if having the option saved as a member variable of the IncludeInserter is fine.

sorry but I am not sure what's the value proposed by this patch in its current form. in https://github.com/clangd/clangd/issues/1247 and other places we've discussed this, i believe the sentiment was towards providing a config option that'll let people customize header insertion style in combination with a directory filter, e.g:

Style:
  IncludeInsertion:
      Directory: foo/
      Delimeter: <
   ... More IncludeInsertion customizations.

Any specific reasons for going with the hard-coded approach? As I don't think we can accept all this extra code to make sure it works for a "handful" projects/users (in the wild I haven't seen many projects that strictly use < includes). The use case you want can still be accommodated by such a generic approach, you can have Directory: / or we can even treat absence of Directory field as an "always" matcher.

My understanding is that a more elaborate configuration scheme has been proposed in https://github.com/clangd/clangd/issues/1367, and the feedback there was (quoting Sam from this comment):

I think this design is very complicated for the value it provides. [...]
For <> vs "" in particular, we do have multiple reports suggesting this. I think we should be open to a either a simple config-based solution

The approach taken in this patch seemed to me to be in line with this direction of a "simple config-based solution".

My understanding is that a more elaborate configuration scheme has been proposed in https://github.com/clangd/clangd/issues/1367, and the feedback there was (quoting Sam from this comment):
The approach taken in this patch seemed to me to be in line with this direction of a "simple config-based solution".

I am afraid this approach is a little "too simple". The intent on @sammccall 's comment there is probably around getting rid of the re-writing of include spellings completely, not for specifying quoted or system includes based on the include spelling.
Even if not, I feel like my argument above still applies. I still can't think of many projects benefiting from always using angles/quotes for include spellings.

My understanding is that a more elaborate configuration scheme has been proposed in https://github.com/clangd/clangd/issues/1367, and the feedback there was (quoting Sam from this comment):
The approach taken in this patch seemed to me to be in line with this direction of a "simple config-based solution".

I am afraid this approach is a little "too simple". The intent on @sammccall 's comment there is probably around getting rid of the re-writing of include spellings completely, not for specifying quoted or system includes based on the include spelling.
Even if not, I feel like my argument above still applies. I still can't think of many projects benefiting from always using angles/quotes for include spellings.

Right - my impression is that most projects that want to force a certain quote style want to do so for certain paths only.

e.g. from #1367:

I'm working on a project where the style dictates that all the includes from our public SDK are included using angle brackets.

A recent comment on #1247:

Since clangd works with little configuring for SerenityOS, we do not need to modify -isystem which is picked up from our toolchain. However, those includes do not include all the headers we include with the bracketed style, so a better way of configuring this is needed.

So while I do think it's important to limit the complexity, a version of the feature that isn't actually usable to most people would just be an attractive nuisance.

I'd suggest something like:

Style:
  QuotedHeaders: "path/to/.*"
  AngledHeaders: "path/sdk/.*"

where the regexes can be single or array, and match any suffix (like Diagnostics.Includes.IgnoreHeader)

Thanks for the clarification and suggested formulation.

I'd suggest something like:

Style:
  QuotedHeaders: "path/to/.*"
  AngledHeaders: "path/sdk/.*"

where the regexes can be single or array, and match any suffix (like Diagnostics.Includes.IgnoreHeader)

@ADKaster are you interested in updating the patch to implement this approach?

azat added a subscriber: azat.May 18 2023, 12:26 PM

Thanks for the clarification and suggested formulation.

I'd suggest something like:

Style:
  QuotedHeaders: "path/to/.*"
  AngledHeaders: "path/sdk/.*"

where the regexes can be single or array, and match any suffix (like Diagnostics.Includes.IgnoreHeader)

@ADKaster are you interested in updating the patch to implement this approach?

It looks like someone has done this and submitted the updated patch as a PR, linking to it for reference: https://github.com/llvm/llvm-project/pull/67749