Address review comments.

Save last status to prevent redundant updates.

Update comment.

Address another round of comments.

Address a round of comments.

Address remaining comments.

Address review comments.

WIP. Need to get around background indexing tricks to make it work.

Also check if deinition comes from unwwanted file (e.g. omit forward declarations).

"Fail" early.

Looks good to me, thank you very much!

Address post-LGTM comments.

Ah, good catch, thank you! Alternatively there seems to be Package_FOUND variable but what your propose is cleaner.

This is not an actual patch, so removing from reviews to reduce noise.

This is a full reproducer of what would work with grpc++ built from source. Seems like the dependencies are pulled by CMake and this way it actually links correctly.

Aww, actually no only LibSSL. I'll have to link all the libraries libgrpc++.a is depending on which means I'll have to find all of them somehow: LibSSL, libcares, libcrypto, abseil... Seems like the easiest way is to check gRPC into LLVM or simply disallow static builds with system-installed libs :(

Easy way to play around some of the things:

Rebase.

When I manually link /usr/lib/x86_64-linux-gnu/libgrpc++_reflection.a to clangdRemoteServer on Linux the following error persists:

This seems to be the related issue for macOS: https://github.com/tensorflow/tensorflow/issues/30117. The proposed solution is hardcoding -framework CoreFoundation :(

macOS error message:

Error log on Linux:

Remove redundant .str() calls.

Handle slash appending in Marshaller.

Oh, thanks!

Resolve post-LGTM comments.

In D84811#2199891, @hokein wrote:

In D84811#2199829, @kbobyrev wrote:

In D84811#2199820, @hokein wrote:

In D84811#2199510, @kbobyrev wrote:

Even despite FileFilter not being fully implemented yet (an issue for a separate patch) I think this change should still be correct and is probably OK to land, WDYT @hokein?

Yes, personally no objection on landing this, but landing it doesn't seem to help much for your remote-index case (STL symbols are not filtered out)?

True, it doesn't filter _all_ of them, but it partially filters some symbols which is still a win I guess :) My rationale is probably that having this blocked on more implementation details would be more reasonable if there was no implementation at all but since it still filters out some symbols this should probably be fine.

It just reduces symptoms, not solving the problem. I think remote-index is also blocked the FileFiltering stuff (we can't launch remote-index without fixing this issue entirely).

As you may have noticed implementing FileFiltering is tricky, I think the indexer here is a good opportunity to test/verify your implementation (comparing the index data before vs after), so my preference would be to implement the FileFilter, test it with this patch together, then finally land this patch after making sure everything works.

WIP, nothing important here.

In D84811#2199820, @hokein wrote:

In D84811#2199510, @kbobyrev wrote:

Even despite FileFilter not being fully implemented yet (an issue for a separate patch) I think this change should still be correct and is probably OK to land, WDYT @hokein?

Yes, personally no objection on landing this, but landing it doesn't seem to help much for your remote-index case (STL symbols are not filtered out)?

Even despite FileFilter not being fully implemented yet (an issue for a separate patch) I think this change should still be correct and is probably OK to land, WDYT @hokein?

@sammccall This is the change I was talking about during the standup: the code looks legit, but BackgroundIndexTests.IndexTwoFiles fails because with the following code path in Background.cpp's FileFilter:

Need tot resolve FIXMEs around shouldIndexFile().

Good points, thanks!

Switch to SymbolCollector::Options::FileFilter that is even more
straightforward.

Hmm, apparently link_libraries is not supposed to be used this way as it adds to linker search path _after_ it's called (from the docs) but this still worked for me on mac before. Hence I was relying on incorrect behaviour, my bad.

This patch is not ready for review yet; planned changes: check if the version is between 1.25.0 and 1.26.0-4 and bail out in such cases.

Resolve most review comments.

In D84939#2184836, @kadircet wrote:

i think you might want to have an helper for creating stringerrors and use it in all places

Client: print debug string for the stream_result, not the ReplyT.

Address post-LGTM comment.

Address all comments.

Fix comment in proto file and rebase on top of master.

Resolve post-LGTM comments.

Thanks!

Use more conservative approach.

Don't use llvm::toString in elog