• run clang-format

There's a test for the new behavior in D50726

LGTM. Let's watch out for possible breakages in any of the clients, though. I've checked VSCode and it seems to be fine with the added fields.

Haven't noticed Alex's comments, sorry for a duplicate suggestion about exiting with failure error code

LGTM. Many thanks for fixing this.

Sorry for missing this one. The biggest concern that I have is about the thread-safety of the API, it's too easy to misuse from multiple threads at this point.

• Use 'auto*' instead of 'auto'

LGTM

Maybe also add a test for find-definition that was broken before? (non-empty preamble -> empty preamble -> bad gotodef that goes to included file instead of the local variable)
To have a regression test against similar failures.

NIT: maybe also note the number of the clangd revision in this change's description

LGTM, but let's land together with a dependent revision to hove some code that actually tests it.

Thanks! LGTM with a few NITs

Sure, LGTM, did not want to block you.

LGTM. Thanks for the change!
Could we add an option to clangd to switch it on? (VSCode does not work, but our hacked-up ycm integration seems to work, right?)

LGTM with a small NIT.

Only a few NITs from my side.
Excited for this fix to get in, have been seeing duplicate in other cases too :-)

I have left a few comments, but wanted to start with a higher-level design consideration.

LGTM. Thanks!

Maybe hard-code the SHA256 checksum directly into the script instead?
Going through the keyservers does not seem to buy us much in terms of security and we certainly don't update the Dockerfiles very often, so it does not take too much time to verify those SHA256 checksums by hand when we do.
Using the gpg was a mistake on my end, it makes things more complicated and less reliable.

The new behavior looks reasonable for interactive tools like clangd, but I'm a bit worried that clang may emit too many irrelevant errors, because it may not cope nicely with those fatal errors, i.e. wasn't tested that thoroughly in that mode.
Nevertheless, I think this is moving clangd in the right direction and if we see too many irrelevant errors, we should look into improving clang to show less of those.

Short summary of the offline discussion: I suggest adding this parameter into the corresponding requests of the index (FuzzyFindRequest, FindDefinitionsRequest) instead of stashing it in the context. Context has all the same problems as the global variables, so we should try to avoid using it where possible.
On the other hand, where this key is stored might not be terribly important if we don't have too many usages and remove it when we can workaround the limitations that we're currently facing.

Maybe split the commit message into multiple lines?

In D50154#1191002, @dblaikie wrote:

What's the motivation for clangd to differ from clang here?

The presentation of diagnostics to the users is different between clang and clangd:

• clang diagnostics are always prefixed with the file, location and severity of the diagnostic, e.g. main.cpp:1:2: error: expected '}'
• In LSP clients (VSCode in particular), the diagnostic message is the first thing the user sees and it looks a little nicer (subjectively) if the first letter is capitalized.

+1 Sam's suggestion of configuring this during initial LSP handshake, rather than as a command-line flag.
We could put it into ClientCapabilities or into initializationOptions. The latter has type 'any' in LSP, so it seems to be most in-line with the protocol to put clangd-specific config options there, but not opposed to having this in other structs as an extension either.

LGTM, with a few NITs.
Thanks for the change!

Thanks! The only major comment I have is about the FixItsScore, others are mostly NITs.

In D48903#1187605, @simark wrote:

In D48903#1187596, @ilya-biryukov wrote:

This revision got 'reopened' and is now in the list of accepted revision. Should we close it again?

It got reverted a second time because it was breaking a test on Windows. The new bit is the change to test/PCH/case-insensitive-include.c, so it would need review again. If somebody else could run the tests on Windows, it would make me a bit more confident too.

This revision got 'reopened' and is now in the list of accepted revision. Should we close it again?

LGTM

Thanks for the change! Overall LG, mostly NITs about the tests.

Seems we have consensus here.
Let's remove the option and just always capitalize the messages.

Maybe we could simply always capitalize the messages? Any cons to capitalizing error messages?
@simark, @malaperle, @ioeric, @hokein, WDYT?

Picking it up since @sammccall is OOO.

Abandoning in favor of George's fix.

LGTM with a small nit. Thanks for the change!

Thanks! LGTM

This change is a bit ugly since it has to account for the value of LLVM_ENABLE_THREADS...
Happy to accept suggestions/alternative ways to fix this :-)

In D49794#1182296, @yvvan wrote:

Clang tidy is not only a standalone tool but also a plugin. It's almost never used this way (but we do that in Qt Creator to combine it with Clazy) and it also seems that some of the recent checks are added only to standalone tool (which is easy to fix).
Being loaded into clang as plugin it does not support most of clang-tidy command line options and is not able to filter system includes. I'm not sure it's possible to fix both of these issues (and probably some other).

In D49794#1182220, @yvvan wrote:

And we already saw, that -isystem is not the best choice for that.

Are you referring to the file-locking on Windows?
Any other reasons why the -isystem trick might be non-ideal?

• Rebase onto head

In D49794#1182078, @yvvan wrote:

I already mentioned in the review inherited by this one that this way covers also loaded plugins with different consumers. So let's assume that driver loads clang-tidy and some other plugins and runs over each file in the project.
It makes sense not to include clang-tidy diagnostics coming from headers, at least for third-party headers (you can still run clang over specific headers themselves to get diagnostics for them).

• Check for PrevDiagsWereReported

• Moved early return into the CanReuseAST branch

• Added a comment

In D49833#1181651, @malaperle wrote:

Was there any objection to this patch? It would have been nice to have this before the branching.

Sorry for the delay, somehow missed this update in my inbox.

Just a few nits left.

Sorry for sending this in instead of waiting for D49158 to land, I hadn't noticed it earlier

• Move the test into an existing file

In D49758#1177747, @arphaman wrote:

In D49758#1174629, @ilya-biryukov wrote:

The mode of operation where compile commands come from the client seems useful, but I wonder if there's any value in mixing it with compile_commands.json and other CDB plugins.
Do you plan to use the overridden commands in conjunction with CDB plugins or do you want the client to exclusively control the compile commands?

The client will control the commands exclusively.

LGTM

Many thanks! Great cleanup. Just a few nits and we're good to go

In D49783#1175688, @simark wrote:

Thanks, that's simple and efficient. I'll update D49267 (to call reparseOpenFiles once again) once this is merged.

• Move OldPreamble.reset() out of the lock, add a comment

Not strictly opposed to this change, but should any reason why the clients can't guarantee they'll send didChangeConfiguration right after clangd is initialized?

• Use log instead of vlog
• Add parens around &&

LGTM

Overall LG. I'm sure it's an improvement overall, just wanted to get some clarifying comments and references, if that's possible.

For the record: this got reverted in rL337850

The mode of operation where compile commands come from the client seems useful, but I wonder if there's any value in mixing it with compile_commands.json and other CDB plugins.
Do you plan to use the overridden commands in conjunction with CDB plugins or do you want the client to exclusively control the compile commands?

In D49523#1174086, @arphaman wrote:

We actually need both mechanisms. I posted the didChangeConfiguration extension to https://reviews.llvm.org/D49758.

if/when we have a native implementation, supporting multiple mechanisms with different layering requirements to get at most a 2x win in watcher resource usage seems like a dubious way to spend our complexity budget.

I guess the only think that might force us to do this is the low system limit on the number of file watches. I do remember rumors that it can be a problem in, but I haven't really seen those on my own, so maybe I'm just pretending.
But I do agree that we shouldn't commit to have yet-another implementation in the long-term, unless we have good justification for it.

In D49267#1173291, @simark wrote:

Ok, I agree that having clangd watch files itself could be necessary at some point (when the client does not support it), but it would have to be configurable. In our case, we have efficient enough file watching in the client, and already watch the files in the workspace. Since the inotify watches are limited per-user, having clangd watch them too means we'll have duplicates, and therefore waste a rather limited resource.

Actually, clangd could simply use the client capabilities. If the client advertises support for file watching with dynamic registration, use that, otherwise use the internal mechanism.

Yeah, sounds reasonable. The limits on the number of watches is definitely something that could bite the editor+clangd bundle, so that's a good reason to use the client watching if that's available.
We want to share some design around this area in Aug-Sep.

In D49267#1171932, @simark wrote:

I guess you mean language client here, not language server. In our case we already have a client capable of file watching, so it's convenient for us to do that (plus, this is what the LSP specification recommends). If you want to make clangd capable of watching files natively, I am not against it, but it's not on our critical path. As long as the file is watched, I'm happy.

Yes, language client, thanks!

LGTM

Wow, that's a huge oversight on my end.
Thanks for fixing this! LGTM