A drive-by NIT ;-)

LGTM

In D52311#1255876, @simark wrote:

I just tried this, this looks very promising! It should help build our outline view in a much more robust way than we do currently.
A nit for the final patch, I would suggest omitting the fields that are optional, like children (when the list is empty) and deprecated.

SG, will do.

In D51725#1255695, @simark wrote:

But I am wondering what to do with the feature that allows clangd to change compilation database path using the didChangeConfiguration notification. In my opinion, it's a bug that it's not possible to switch back to use no explicit compilation database path, so I'd still like to get this patch merged. Or, if we decide this is really not useful, then it should be removed. I don't think we should keep the feature in the current state, either we fix it or remove it.

Fair point. The original patch looked scary mostly because of Optional<Optional<...>>. Many thanks for clearing up your use-case, though. I thought Theia was exclusively using custom directories for compile_commands.json, I didn't know the user had an option to turn this behavior on/off and configure directories explicitly.
I would question the usefulness of the option if Theia can live without it, so I'd vouch for removing it. @sammccall, WDYT? Especially wrt to auto-indexing and it being a customizable build directory?

Putting a stamp, assuming is answer is "yes, it's a legacy way to specify all scopes"

Overall LGTM, just a quick question to make sure I get what's going on.

LGTM and a few NITs.

LGTM

This is awesome! Can't wait for it to land!

I would call it a requirement instead of an assumption. The replay must be exactly the same (even the file stats and reads). If Clang reads the file in replay which was only stat()ed during compilation, it indicates non-determinism or something wrong (in clang or FS).
We currently deal with such files by adding empty buffers for them based on this assumption/requirement only.

A drive-by comment.
Would it be cleaner to pass this information from clang? Relying on completion label seems shaky.

• Remove debug formatting
• Update snippet for no-arg case with the suggested changes

This seems very clever, but extremely complicated - you've implemented much of C++'s conversion logic, it's not clear to me which parts are actually necessary to completion quality.

Clearly the model that supports C++ conversions is something that will improve code completion quality.
I do agree it's not trivial, but would argue we at least want:

• qualification conversions (i.e. adding const)
• user-defined conversions (e.g. operator bool is commonly useful think)
• derived-to-base conversions (Derived* should convert to Base*)

In D50171#1236792, @mgorny wrote:

@ilya-biryukov, gentle ping. I'd like to patch this for 7.0.0 in Gentoo. Do you think my patch would be good enough, or do you expect to submit something else soonish?

Thanks! LGTM!

Posted to make sure it's visible that I've started doing this.
Still need to update the tests and check for the capability from the client (and fallback to SymbolInformation if client does not support the new implementation).

In D52261#1240143, @yvvan wrote:

I tried that first but did not I find a way just to copy an expression (we basically need the same expr for such case). Do you know how to properly generate a copy of expression or some other way to get the same expression?

Also not sure about the trick:

• Would be surprising to see the "ms" instead of "mbytes"
• Time tends to vary between runs, so using Google Benchmark's capabilities to run multiple times and report aggregated times seems useful. Not so much for the memory usage: it's deterministic and running multiple times is just a waste of time.

It's the responsibility of the caller to provide a corrected expression, this keeps the completion function simpler. I suggest we stick to this contract.
We do something in getLangOpts().CPlusPlus case to create a corrected expression, let's just implement a similar fallback for C at the single callsite that we have (ParsePostfixExpressionSuffix).

Ranking-related code should be moved to Quality.cpp

Need to store the types in RIFF format too

The implementation might look a bit scary, please feel free to ask for comments/clarifications!

LGTM

LGTM

Maybe commit only an option to enable function arg snippets for now (found myself wanting this option today :-))? The fixes would also be nice, but since they never work...

LGTM

Maybe add a test?
There is test/Index/complete-pch-skip.cpp that checks similar things for AST completions, adding macros there should be trivial.

LGTM

Seems like this optimization is not worth (yet?). As soon as we get more proximity paths (and hence more OR iterator children) that might make sense.

WDYT about storing all the elements with the minimal doc-id outside the heap? I.e. we can pop all elements with the minimum doc-id on 'advance' and iterator creation.
That way we could potentially regain the improvements we've seen in the first version.

Overall LG, see the major comment about running without the preprocessor.

Sorry for the late response.

Landed the patch as r342363.

~1% increase in memory usage seems totally fine. Actually surprised it's that small.
Overall LG, just a single comment about extra copying. Thanks for the change, looks like a great improvement!

Amazing, can't wait for it to land!

Neat, I was unaware that single-file mode completions actually work nicely in that case.
LGTM!

In D51747#1233499, @kadircet wrote:

Np, not planning to land it before we figure out the issue with vim can't handling lots of diagnostics anyway.

LGTM

In D51725#1233254, @simark wrote:

I am not sure I understand correctly your last point. Of course, when restarting clangd, we need to send again a didOpen for each file the user has currently open in the editor. But that should be it?

Yes, this and any initial configuration settings.
My personal experience is from VSCode, which does not call addDocument IIUC. This results in clangd producing errors about running actions in non-opened files.

LGTM

Great improvement for such a simple change!

LGTM with a nit.

Will let Kadir finish the review, consider lgtm'ed from me ;-)

but the thing you're spending the CPU on is checking for cancellation...

Unless checking for cancellation is really cheap (which is doable, I think).
We should probably hit some of those in practice before doing something, though.

If this setting exposed directly the users in Theia or is it something that is exposed via a custom UI (e.g. choosing named build configs or something similar)?

A small drive-by comment.

In D51971#1232064, @kbobyrev wrote:

Apparently, the parsing seems to be wrong; I should fix that first.

Wow, this is getting somewhat complicated.

Thanks for the comments everyone!
Will land this tomorrow, since I'm buildcop this week anyway.

You beat it to me, but I thought we didn't use indexer as it could be confused with the actual indexing?

• Rename to clangd-indexer

clangd-indexer looks nice and short. @ioeric, WDYT?

+1 to making this hidden. Users (most of them) shouldn't care about it.
Is there any motivation on why we someone would want to disable it currently? Maybe we want to deprecate it and remove it completely at some point?
I could only think of being as close as possible to sema completions, but that doesn't seem like something that gives better UX (folks from Apple might be interested in that, but not sure).

We've used the new name internally before and now that we're testing this, we need to be consistent.
The 'clangd-symbol-builder' looks like a better choice, so I'm pitching changing upstream first.

• Rebase, updated the added test

Will take a closer look. Thanks for finding the offending revision.

LGTM. Thanks for the fix!

In D51747#1229066, @sammccall wrote:

A few thoughts here:

• does CDB describe user or project preferences? unclear.

Agree, it's a mix, defaults are from the project but users can add extra flags.

+1 to adding an option to drop arguments from snippets and removing the option for the fixes.

LGTM