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.

We decided to go with a different solution:
https://reviews.llvm.org/D50452

Hi, we decided to go with a different solution:
https://reviews.llvm.org/D50452

We decided to abandon this direction in favor of simpler solution.
https://reviews.llvm.org/D50452

I like that idea! It looks like it's living in a wrong place anyway as it doesn't need access to any of implementation details (private members) of DiagnosticID. I would still like to preserve it as a function so this block of code has clear semantics and interface.
How about I refactor it to a static free function in Warnings.cpp?

Hi Sam, we are still discussing internally how to fit clangd and XPC together. Please bear with us and ignore our patches until we decide.

BTW it looks like we already had kind of support for compilation command before (extra flags).

BTW Just for the record - I put the rebased & updated patch here: [clangd] XPC WIP https://reviews.llvm.org/D49548

BTW Just for the record - I put the rebased & updated patch here: [clangd] XPC WIP https://reviews.llvm.org/D49548

BTW Just for the record - I put the rebased & updated patch here: [clangd] XPC WIP https://reviews.llvm.org/D49548

Hi Sam, we discussed a bit more with Alex offline. Ultimately we don't mind using JSON as part of the generic interface but we'd like to optimize specific cases in the future (thinking only about code completion so far) which might need to "work-around" the abstraction. What is your opinion about this - does it ruin the whole point of having the abstraction or do you consider pragmatic to have abstraction covering most of the functionality with maybe couple exceptions? I kind of assume (please correct me if I am wrong) that you might have done something similar with profobuf interface so we're definitely happy to discuss.

Alex, I am just wondering if we shouldn't rather create another implementation of GlobalCompilationDatabase interface (something like InMemoryGlobalCompilationDatabase), add it to ClangdServer and use it as the first place to be searched in ClangdServer::getCompileCommand(PathRef File). What do you think?

Hi Marc-Andre, what is a structure of data you are passing as parameter of didChangeConfiguration message? All we need is to pass per-file compilation command to clangd. Maybe we could send didChangeConfiguration message right after didOpen.

Sam, just out of curiosity - would it be possible for you to share any relevant experience gained by using porting clangd to protobuf-based transport layer?

LGTM with just a nit about comment wording.

Hi Sam, I am definitely open to discussion about the right abstraction.

Hi Sam, thanks for your feedback!

Hi Sam, no worries! Thanks for making time to take a look! I am going to rebase all my patches on top of JSON library moved to llvm/Support and process your comments.

Ping. Added reviewers suggestion.

Two changes in test client based on internal review by XPC folks:

• Removed cleanup code at the end of main() as dispatch_main() never returns.
• Removed check for conn as xpc_connection_create() is guaranteed to succeed.

Followed by these two patches:
[clangd] JSON <-> XPC conversions
https://reviews.llvm.org/D48560
[clangd] XPC transport layer
https://reviews.llvm.org/D48562

Thank you for the explanation!

I don't particularly like that between setting the DeclContext (SemaTemplateDeduction.cpp:3814) and actually using it (CheckAccess() in SemaAccess.cpp:1459) are some 20 stack frames but it looks like you already tried fixing this "locally" in your initial approach.

After some internal discussion we agreed that we can simplify things and get consistent behaviour by using dot everywhere in diagnostics.

LGTM but my review was fairly superficial.

We reconsidered this in light of the policy - thanks for pointing that out Richard!
Just to be sure that I understand it - the policy is meant for CLI and not serialized diagnostics, right?

Eric, thanks for the context. I am now clarifying internally and if possible would like to simplify the whole format business by using just one delimiter everywhere. Would that make sense to you or do you think we should respect delimiter used in sources?

Sorry for me being dense here - since the output format is determined by input source code there's more work to do.

Hi Bruno, I just noticed couple of implementation details.

I am not 100% sure it's the best thing to set printing style at the point of parsing version tuples but I am not sure it's bad either. Unless someone convinces me otherwise I would rather not do any major changes.

Fixed the test. It turned out that version component separator for availability is set during parsing which is why all other tests (including my bad one) are passing.

Sorry, my bad. I tried to get rid of dependency on Foundation.h and didn't check the test is relevant for the fix after that.

Volodymyr, could you please confirm that the non-anonymous vs non-inline logic makes sense to you?

Addressed comments + proposal for refactoring of the namespace-related logic.

I forgot to mention review in commit message.