Thanks a bunch for fixing this Fangrui!

Hi Ilya, I got here from reading your other patch (https://reviews.llvm.org/D56611). I'm wondering if we could make those range utility functions more understandable.

Hi Ilya, this seems really useful for people learning how to implement their custom actions!

Hi Erik, this looks neat!

@arphaman or @sammccall, could you please take the final look?

Fixed the synchronous handling of events.

added one more assert

I was thinking the same (that we might be able to make the detection more precise) but I got the impression these cases are handled later (L524 and below).

@hokein I think you are correct. I meant that it would be possible to make LSP more generic by using trigger strings instead of trigger characters..
@ilya-biryukov Yes, that's true. I would still expect some performance gain in more restrictive filtering on client's side - not sure how big though. Anyway, seems like LSP folks think character + trigger context is good enough. For example here:
https://github.com/Microsoft/language-server-protocol/issues/138

Hi Alex, thanks for fixing this.

This looks like a work around LSP imperfection indeed.

In D55363#1337376, @hokein wrote:

In D55363#1336315, @ilya-biryukov wrote:

Maybe consider sending an update after some period of time, e.g. 0.5s? (I expect this particular strategy to be a bit complicated and out of scope of this patch, so another alternative is to simply not send them to the clients in the first version). WDYT?

Sounds fair, keeping status bar blinking seems annony to users. Added a FIXME.

Since AFAIK Sam is off until the end of the year I am adding more reviewers.

Addressed most of the comments.

Going to split SymbolID.h as a NFC commit per Alex's suggestion and land this.
Thanks everyone!

Finished tests and fixed operator<<(SymbolDetails) for containerNames that aren't part of fully qualified name.

Morphed into https://reviews.llvm.org/D54799

Adressed last comments + initial unit tests.

Removed empty line noise and fixed doxygen annotation.

Addressed comments from the reivew.

Thank you very much for the review Sam!

Hi @hokein, I am just keeping up to date with changes.

Multiple symbols per location.

Couple minor changes based on discussion.

• Move SymbolID to index/SymbolID.h.
• Rename in ClangdServer - drop the verb from method name.
• Remove conditional return in XRefs.cpp.

In D54799#1306585, @sammccall wrote:

So I think both SymbolID and USR are optional.

No problem.
I am just wondering if it make sense to include any symbol with empty name, empty USR and no ID in LSP response. I assume either clangd or our LSP client (and hypothetically others too) will have to filter these out. But it might violate consistency with other LSP methods. Anyway, if we agree on filtering such symbols either in getSymbolInfo() or ClangdServer::getSymbolInfo() I am happy to do it, if not it's fine with me.

Changes based on review.

Hi Sam.

Hi Kadir, I have one small nit otherwise LGTM.

Adding @rjmccall and @doug.gregor who might have some insight.

LGTM. Thanks for the patch!

Ping.

Could you please add a test? I'd suggest minimizing the testcase you linked and placing it to clang/test.

In D54428#1297147, @sammccall wrote:

A question about the high-level build target setup (I don't know much about XPC services/frameworks, bear with me...):

This is set up so that the clangd binary (ClangdMain) can run unmodified as an XPC service, all flags and options are still respected etc.
At the same time, the framework plist seems like it is designed to invoke the clangd binary in a very specific configuration (no flags will be plumbed through).

Is it actually important that the clangd binary itself can be used with XPC, rather than defining another binary that spawns a ClangdServer+XPCTransport with the right config? If you strip out all of ClangdMain that's not related to flag parsing and options, there's almost nothing left...

This looks reasonable to me but asked people with more context to take a look.

Correction - I asked offline about our intended use of USR in LSP and it seems we might want to receive it as part of other responses too. Nothing specific for now but it's probable that it won't be just this singular case.

We are also discussing creating separate method as we'll likely want to add other data to the response in the future. Would you prefer USR not to be in the standard part of LSP but only in our extension?

Hi Sam!

One more thing - shall I create new client capability flag for this?

Rewritten tests to shared implementation different cases.

LGTM Thanks for fixing this!

LGTM and it seems like all of Eric's comments were answered too.

I tried to move the getNearestOption() to it's only client - EmitUnknownDiagWarning() but it turned out to be a significant change because of clang/Basic/DiagnosticGroups.inc use in DiagnosticIDs.cpp.
I suggest we leave that for eventual future refactoring since it would blow up scope of this patch a lot.

Allright, I will remove destructor from clangd completion results which solves this particular issue.

Sorry my bad. You are right, we aren't showing destructors in clangd for normal classes. The case where I noticed is kind of a corner case with template class.

You might be right - I am assuming here that we want consistent behaviour between constructors and destructors.

Hi Volodymyr, do you think you might take another look?

I got some test failure with the patch, still investigating the issue.

I tried to come up with some input that breaks current implementation so I could add the test. Problem is that invalid memory read doesn't guarantee deterministic crash.
E. g. with this input the current implementation is definitely reading way past the buffer:

SmallVector<char, 1> IgnoreMe;
const char* Foo = "foo%";
const char* FooEnd = Foo + 4;
Diag.FormatDiagnostic(Foo, FooEnd, IgnoreMe);

Hi Volodymyr,
Thanks for the feedback - interesting points!

Sorry for the delay - I was busy with other things for past two weeks.

Oh, I thought that what everyone wanted was test-specific behaviour. I like both approaches you propose much more!
If we go for the generic option we can effectively start "checking stderr" in tests by using the flag. That would be nice.

Hi Alex, nice work!

Adding Volodymyr who landed related patch recently.

I can see the value of getting more information in case of unexpected test behaviour but I still don't really like to have separate codepath for testing purposes.

I think that by introducing different codepath for testing purposes we might end up with fewer bugs in tests but the actual production code could become less tested. Actually, even the -lit-test itself might be not the theoretically most correct approach but I do see the practical reason for it. In general I'd rather keep the testing specific code to a minimum though.

Hi Greg, this looks interesting! I got curious about your patch since I am dealing with another protocol from the same "family" - LSP.