Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enhanced clangd options UI #122

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Conversation

rapgenic
Copy link
Contributor

In this PR I exposed a few clangd options in the vscode UI for an easier configuration by the user.

For backwards compatibility, the preexisting clangd.arguments setting has precedence over all the new options. This way, existing projects/configurations that are already working won't change behaviour without the user knowing it.

I chose a few (for me) common options to avoid adding too much clutter (of course I can add more if you think they're important):

  • --compile-commands-dir by default empty
  • --query-driver by default empty
  • --background-index by default true
  • --clang-tidy by default false
  • --cross-file-rename by default false
  • --header-insertion by default iwyu
  • --limit-results by default 100

Default vaues have been chosen according to clangd default values.

All options are used only if the current clangd binary supports them, by checking its major version. Versions before 7 are not checked for compatibility.

Note that I could not test it on windows/mac machines and with older clangd versions.

Fixes: #50

@rapgenic
Copy link
Contributor Author

In the latest commit I have also added a warning popup that advises to update the configuration in case the clangd.arguments settings contains conflicting options, with a button to open clangd settings.

@njames93
Copy link
Contributor

njames93 commented Dec 18, 2020

Can't speak for earlier versions, but since clangd-11, --clang-tidy and --cross-file-rename default to true.

@rapgenic
Copy link
Contributor Author

I am quite sure at least --cross-file-rename was false on previous versions, however I'm not sure what can be done about that, because there is no way to specify a different default setting according to the clangd version.

I think that the default settings shipped with the extension should be the latest ones: at most a feature which was still experimental in previous versions would be enabled.

Plus, it's very easy to install the latest version of clangd from inside the extension, so maybe that should be the encouraged behaviour to lower the risk of such small incompatibilities.

@sam-mccall
Copy link
Member

Thanks for looking into making this more friendly!
I'm not really sure about this approach, for a few reasons:

1: Flags are specific to a clangd version, but the extension is not, and we can't vary vscode settings based on version. This means:

  • we're in danger of offering people settings that don't exist. If set, clangd will "crash" (exit after failing to parse the flags)
  • if we try to avoid this, then we can't offer useful settings from recent clangd versions
  • the extension-default settings will be nonsensical and confusing for some users, it's very common for us to switch defaults between versions, to roll new features out.

2: This diverges VSCode configuration further from other editors. We need to maintain this set of vscode-specific mappings (and decide where to draw the line, as there are many flags). Documentation for these options must refer to the generic flag mechanism (confusing vscode users) or become cluttered with editor-specific instructions.

3: Command-line flags have various weaknesses (backward-compatibility, limited structure, readability, changing in a running instance, limiting to certain files, storing in version control) that are addressed by using a separate config file. We're slowly moving to this model, and it will be the preferred way to set the things flags are commonly used for today. Building more infrastructure around command-line flags hinders this migration.

@rapgenic
Copy link
Contributor Author

Hi @sam-mccall, thank you for your review! This was, as you correctly undestood, an attempt to make more friendly the configuration of clangd, given that right now it is a little confusing, mainly for two reasons:

  • Options are not easily known/discovered
  • Keeping a default set of options (in the global settings.json file) and a per workspace one (in the .vscode/settings.json file) does not allow the merge of the options, as the clangd.arguments options gets always overridden by the workspace one, which forces to copy the default options in the workspace as well.

By separating the different flags we can easily provide documentation and per option override in the workspace settings.

Of course your doubts cannot be ignored:

  1. This was my main concern, too, which I partially solved by adding a version check to the binary, however this might not be enough for some development releases and of course I could not test all the versions myself. This might be solved by forcing all the users to install automatically the latest clangd version from the extension (In my opinion, this would also be useful to avoid bug reports caused by people using old versions and would provide a more extensive user base to test the newest releases). Of course this is an important change and I do not suggest it to be made just to solve this issue.
  2. That's right, did not think about that. Even though that's not that big of a difference, as this extension would provide just a little more scaffolding around the command line arguments. I agree that the documentation, with the reference to the flag and the latest version supported is a little confusing.
  3. Totally get this and I agree with that, it would be definitely more versatile and open to other tools.

It is clear that the direction of the project is towards the config file, so in my opinion this PR might be useful just as a transition from the command line flags to the config file, to make the extension a little more user friendly in the meantime. However as you pointed out the advantages might not be worth the disadvantages it could introduce. Probably if all users used (or were forced to use) the last version of the server the advantages would win. However as I said this is your decision and should not be related to this PR.

If you think this is worth giving a try, I am more than happy to listen to any advice to improve it, otherwise feel free to close the PR if you think it deviates from the project direction.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Split "clangd.arguments" setting into multiple options
3 participants