[clang-tools-extra] issue-63565: community requested small QoL fix for more configurabili… (PR #108005)

via cfe-commits cfe-commits at lists.llvm.org
Sun Sep 22 00:41:18 PDT 2024


MK-Alias wrote:

> I think it would be a nicer user experience if the config file took precedence over compile flags...

The whole idea of command-line arguments is to override defaults at execution time. Config-files should be persistent part of the config, and to override the current config, you can use - non persistent - command-line arguments... That's the whole idea behind it is so you can run a command with a _certain intent_, The other way around will render the command line useless, or might have unexpected behavior. (say: supplying a command line argument and it then not doing anything)

> (this comment remains to be addressed)

I really don't know what you mean [here](https://github.com/llvm/llvm-project/pull/108005#discussion_r1769703751)!? can you elaborate? what do you want me to do?

I'm pretty happy with the patch, as-is. Only the boolean on the command-line `EnableFunctionArgSnippets` is [lacking features](https://github.com/llvm/llvm-project/pull/108005#issuecomment-2358650558). I'm checking the `false` state of it, which means that the `true` state is ignored. I've now changed it to a `int` value with the default value of `-1`. So if the value is `-1`, i can ignore it in the `FlagsConfigProvider`. So now both `false` (0) and `true` (1) are properly handled.

> but it confuses things when "git blame" says that the last commit that touched some unrelated....

And it's not confusing if "git blame" says that this same part of code is there in a "clang-format commit". Instead of it's original? The world is simply not perfect. The git blame is going to change these lines anyways.

[A choice have been made to use clang-format](https://llvm.org/docs/Contributing.html#how-to-submit-a-patch). The source currently is not properly formatted because the previous commits where not properly formatted, or a newer version of clang-format has a different interpretations of how to format the code. Me now having to first format the code to comply to the clang-format requirement, and then having to unformat other parts of the source is a absolute annoyance! And also the reason why the [clang-format check](https://github.com/llvm/llvm-project/actions/runs/10924307640/job/30480533009?pr=108005) is now failing. The irony is that people choose to use a code-formatter to not have to discuss or deal with code formatting, and then we now have to discuss the code formatting.

Code-formatters have their pros & cons, and this one of the cons, and personally I would argue that this issue is just "collateral damage" of using a code-formatter. Things like these can and will happen from time to time, if the code isn't properly formatted, or the formatter [clang-format] gets updated. It's simply inherit of the choice of using a formatter.

I will commit the newest version of the patch, which will have the mentioned int value on `EnableFunctionArgSnippets`. It will be properly clang-formatted. And if you have no other critic/insights and agree with merging this version you wave three options.

A: Merge it as is (recommended: this follows the [guide](https://llvm.org/docs/Contributing.html#how-to-submit-a-patch))
B: Clang-format the code-base (or at least the affected source files in this patch) and merge that. And then merge this patch. If there are any conflicts then - there will probably not be, because my version is clang-formatted - I will happily pull the new version and make a new patch if necessary!
C: make a local copy of my patch, reconfigure it to your licking then merge it.

Effected files in this PR are:
```
clang-tools-extra/clangd/ClangdServer.cpp
clang-tools-extra/clangd/CodeComplete.cpp
clang-tools-extra/clangd/CodeComplete.h
clang-tools-extra/clangd/Config.h
clang-tools-extra/clangd/ConfigCompile.cpp
clang-tools-extra/clangd/ConfigFragment.h
clang-tools-extra/clangd/ConfigYAML.cpp
clang-tools-extra/clangd/tool/ClangdMain.cpp
clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
```

So if you which to first clang-format before merging, you technically only have to do these.

https://github.com/llvm/llvm-project/pull/108005


More information about the cfe-commits mailing list