Added the test.

• Added a test for preamble rebuilds.

• Invert flag, remove the default.

In D47272#1110958, @ioeric wrote:

Would it be possible to add some integration tests for file index plus preamble?

I have only a few nits left, otherwise looks good. Thanks for making this change! Really excited for it to get in finally!

But I feel it's a bit odd that completion and workspace symbols would be inconsistent.

It does not seem that odd to me. Completion is something that should follow the language rules more closely, i.e. we don't want to deviate from sema completions too much. While workspaceSymbol is a user-facing feature and we probably want users to type as little as possible there.
E.g. not showing namespace_name::A in completion seems bad to me, since people are used to referring to the enumerators this way and sema completion gives you those results.

• Simplify the change by boosting any Decls that have a redecl in the current file

After an offline chat: we decided to boost sema results that have any decls in the main file. Even though it introduces some unwanted inconsistencies in some cases (e.g. see the comment), most of us agree that's a very simple implementation that does boost useful things, including stuff from association header.
To get better results, we need to design and build the real proximity scoring that handles associated headers and other things.

LGTM.

• Reimplemented LRU cache with shared_ptr and weak_ptr.

• Added forgotten renames

@malaperle, a slight offtopic. I was wondering which completion options do you use for clangd? Defaults?

In D47272#1109909, @malaperle wrote:

We do not to rely on symbols from the main file anyway, since any info hat those provide can always be taken from the AST.

I'll be adding those soon for workspace symbols... And also for document symbols.

In D46943#1109199, @ioeric wrote:

• Symbols store their paths as URIs ⇒ we need to parse them in order to apply heuristics. We could avoid that by writing a version of header-matching that also works on URIs, but that would mean more complexity.

Yeah, this is a bit unfortunate. It's probably OK to parse URIs; they are not that expensive after all (we might want to do some measurement though).

Yeah. I really wish we had microbenchmarks for things like completion.

I'm not sure if we have tests for that, but I remember that we kept the enumerators in the outer scope so that completion could find them..
Am I right that this patch will change the behavior and we won't get enumerators in the following example:

I've added an initial version of testing for the matching header and wanted to get feedback before proceeding further with tests and other changes.

• Move helpers tha switch header and source into separate files.
• Also uprank items coming from the matching headers

In D47065#1104730, @ioeric wrote:

Should we also change the behavior in dynamic index?

LGTM with a minor naming suggestion.

Another alternative that I've considered was evicting the ASTs from memory after a certain period of time, e.g. after 30 seconds of inactivity for some file. This might be simpler and also cover the use-case of speeding up multiple code navigation requests (findDefinition/documentHighlight) in a row.
Yet another thing that came to mind: walking over all of the AST to find reference under the cursor is terribly inefficient, we should be able to build a cheap data structure that speeds up this operation. Maybe we can even store enough information to not need the AST for navigation at all and build it only for features like refactorings.
@sammccall, let me know what are your thoughts on all of this.

In D44954#1103664, @malaperle wrote:

It's probably better to consider this in a future patch. Maybe something like the first suggestion: vector::push_back and match both. Otherwise, I would think it might be a bit too verbose to have to spell out all of the specialization. Maybe we could allow it too. So... all of the above? :)

This certainly does not have to be addressed in this patch. Just wanted to collect opinions on what the behavior we want in the long term.
My thoughts would be towards allowing only vector::push_back and make it match both push_backs: in vector<bool> and vector<T>.
Other cases might work too, but I wouldn't try implementing something that matches specializations. It's just too complicated in the general case. This will only be used in workspaceSymbol and it's fine to give a few more results there...

• Move tests to QualityTests.cpp
• Fix findDecl() assertion on redecls of the same thing
• Fix file comment of TestTU.cpp

A few questions regarding class members. To pinpoint some interesting cases and agree on how we want those to behave in the long run.

Sorry, a few more things and we're good to go.

Rebase onto head, fix merge conflicts

LGTM

• Added tests that all comments are being parsed.

• Fix code after a change in deps (getFormattedText now needs a SourceManager instead of an ASTContext)
• Address review comments

LGTM

• Rebase onto head and remove \brief from the comments, it's gone upstream now
• Rename CurrentArg to ArgIndex

Maybe move the tests back to this revision?
There are tests for code completion that don't depend on libindex or libclang and use clang -cc1 directly, i.e. tools/clang/test/CodeComplete. This would require adding a flag to clang and patching up PrintingCodeCompleConsumer to print the fix-its, but that should be easy. Moreover, it's useful to have the option to show those completions in clang anyway.

• Removed the overload that accepts ASTContext

LGTM

This LG, presuming there are no semantic changes here, just moving the code around.
Also see the nits about IncludeStyle that seems to be in the wrong change...

LGTM

LG. Just a quick comment about possibly unrelated change.

LGTM

In D41537#1097601, @yvvan wrote:

I hope this review will be over some day :)

• Address review comments

Here :)
http://www.sidefx.com/docs/hdk/_s_o_p___bone_capture_lines_8proto_8h_source.html

Didn't take along to find an example. Thanks for digging this up. That looks like a good enough reason to not apply proto-specific filtering based solely on the filename...

• Simplify test code with SourceManagerForFile.

LG when all the dependencies are done

So, the first line of the file generated by proto compiler seems to be something like this:

// Generated by the protocol buffer compiler.  DO NOT EDIT!

In D46751#1095894, @malaperle wrote:

Can there be an option for this? This seems very library specific and could break other code bases. Ideally, there would be a generic mechanism for this kind of filtering, i.e. specify a pattern of excluded files or symbol names. But I understand this would be cumbersome because you want to filter only *some* symbol names in *some* files, so it would be difficult for users to specify this intersection of conditions on command-line arguments, for example. I think this needs to be discussed a bit more or have this turned off by default (with an option to turn on!) until there is a more general solution for this kind of filtering.

This change is big enough to be somewhat hard to grasp.
Maybe we could split it into two, to make it easier for review?
I.e.

• Extraction of IncludeStyle into a separate subclass.
• Moving stuff around without other refactorings.

LGTM, just a few non-blocking NITs with questions.

This LG, but we should first roll out the additionalEdits change.
Aren't the dependencies reversed in the dependency stack, i.e. this change should be the last one?

In D42966#1085303, @mikhail.ramalho wrote:

Hi,

Where do virtual files come from in the first place?

From the linemarker:

• Moved tests to clang

• Renames and other comments
• Don't include brief comments in signature help either, comments there are also handled by the code completion code now.

• Fixed infinite loop with comments that contain doxygen commands

@arphaman, friendly ping. In case you're the wrong person to review it, I'll try to find someone else.

• Move unit tests from clangd code to AST tests
• Assert locations are valid
• Address review other comments

The new uploaded diff has lots of unrelated changes to clang-tidy, clang-move, etc...
Looking at commits, it seems arc diff was called with the wrong base commit...
Could you please reupload the change?

The change makes both testing and scoring code better.
Even though those are largely independent changes, perfectly happy to review them together.

LGTM

I somehow missed the review email, sorry for the delayed response.

Sorry for the delay. I really like the direction of this patch!
What's left is defining the semantics of corrections more thoroughly to make sure we don't have tricky corner cases that users of the API can't deal with.

Sorry for the delay

LGTM

In D42966#1085257, @mikhail.ramalho wrote:

Should we ignore linemarkers and use filename + offset of the real file?

I would say "yes". Let's not rely on linemarkers, unless we can explain why that's a good idea.

This is really a corner cases that users might not need to know about. But an example is:
Code: "#include <a>" (without \n at the end). After inserting <x>, #include <a>\n#include <x>\n (this is good). However, if you insert another <y>, the code would become "#include <a>\n#include <x>\n\n#include <y>\n" (note the double newline!).

Even though that's rare in practice, it seems like an actual bug.
How hard would be it be to fix it?

the patch LG and fixes the problem of llvm namespace not showing up in completions from the static index.
Let's add some tests and ship it!

This change LG as an extraction of the helper functionality to be reused in clang, clang-tidy, etc.
However, I feel there are potential improvements both to the underlying code and the new APIs that we could make.

In D42966#1079438, @mikhail.ramalho wrote:

The virtual file is actually registered in the SourceManager but the
FileEntry for it is NULL (USRGeneration.cpp:33), which forces printLoc to
return true (USRGeneration.cpp:38) and the USR is not generated.

I still don't get why the file is virtual. Looking at the code in SourceManager, presumed location uses FileEntry of passed location (actually, its expansion location, but that shouldn't matter) and then translates line numbers according to the #line directives in the file.
So the question is: why is FileEntry null for original location, but not null for PresumedLoc?

OK, I've rechecked this change. I don't see any obvious mistake :)

I think I got to the bottom of it. We didn't expect a big win, because we expect people to not put their non-template code into the header files. Which is probably true.
The problem with our reasoning, is that this patch also skip bodies of non-template functions inside template classes, e.g.

Could you upload the patch with full context?

• Remove tryLexCommands(), call into helper that parses commands directly
• Addressed other review comments