(friendly ping)

(ping) Does this make sense or are more adjustments needed?

Use ArrayRef instead of SmallVector to avoid copy (also fix 2 typos in the comment below)

(Note that this diff is stacked on D147846)

• Move to free function in CodeCompletionStrings.h
• Add back comment markers before parsing (Index stores comments without markers, but the comment lexer expects them)

Thanks for reviewing!

In D142014#4106088, @v1nh1shungry wrote:

Thanks for the review! @kadircet

Would you mind having a look at if there're any concerns about the current code change, @nridge, @tom-anders, and @adamcz? Thanks a lot!

Move EnclosingNamespace logic to scopesForIndexQuery, add FIXME

Address review comments, add regression test

Factor out logic into helper function

Address review comment: Introduce new AccessibleScopes variable

Add test for expression, improve presentation for signature with unnamed parameter

In D140775#4021634, @nridge wrote:

Do you want to add a simple test case for a non-literal expression? Something like hovering over the + in 2 + 3 should work.

Thanks both of you for reviewing!

Refactor getHoverContents to add CalleeArgInfo for all kinds of expression

s/llvm::Optional/std::optional/ for containerName field

LGTM!

Rebase, fix review comments

Thanks for the detailed feedback! Unfortunately, I’m sick right now, so I probably won’t be able to give a detailed answer until after Christmas.

In D138028#3957139, @v1nh1shungry wrote:

A gentle ping? Sorry if it bothers you.

Add end-to-end test

In D138425#3941206, @sammccall wrote:

In D138425#3941183, @v1nh1shungry wrote:

Yes, there is a common situation where people use a meaningless template parameter name, but I think the same for functions.

I don't think it's the same: my wild guess would be 30% of function params have useless names, and 90% of template params do.
If this were accurate, it seems function param hints are usually (potentially) useful, while template params are almost always useless.

In D137894#3940600, @sammccall wrote:

Since this is a protocol extension, you might want to add an end-to-end test: variant of clang-tools-extra/clangd/test/xrefs.test (xrefs-container.test? to avoid complicating all the existing tests).

Address comments

Thanks again! Landed this with your suggestions incorporated.

In D137909#3933730, @sammccall wrote:

Thanks! sorry about continuing to drift this patch, but this looks like a great improvement.

No problem! I wasn't quite happy yet with the previous version of this patch either, really appreciate the continuous feedback!

Use std::pair for returning payloads along with points/ranges. Also, refactor internal logic.

Fix clangd::Annotations interface, now also has pointWithPayload etc.

In D137909#3930043, @sammccall wrote:

What do you think about $name(payload)^ for the general case, with $(payload)^, $name, and ^ as abbreviated forms? I don't think the lack of brackets on names hurts a lot if they stay simple, and if they're "just labels" that don't have to carry arbitrary payloads then they can be simple.

Keep name syntax, add optional payload instead

Fix one test I missed

${} instead of $()

LGTM, but I'll give @kadircet a chance to review as well

In D137909#3924587, @sammccall wrote:

This seems pretty nice (I do find foo$long_word^bar a bit hard to read). I think I'm in favor, my concerns would be that for a small benefit we're:

• adding some extra complexity people have to understand
• providing more ways to write something, so more bikeshedding opportunities
• encouraging people to embed structured information in the annotations, in a way that won't be very readable *and* isn't well-supported by the API

Can you give examples of where you want to use this?
(I think we should *probably* accept this regardless, but tests want to iterate over payload-name pairs or things like that we should think a couple of steps ahead)

In D137040#3924115, @nridge wrote:

Thanks!

Clean up test

Fix test

This looks really cool :)

In D137650#3919089, @kadircet wrote:

sure i am happy to land, @tom-anders do you have anything else to add ?

Hmm I added the test for the flag to Sema, but now we kinda have the same test case duplicated in sema and clangd tests - I guess for clangd we now actually only have to test that the SnippetSuffix is cleared when FunctionCanBeCall is true, but I don't see an easy way to somehow inject fake Sema results into CodeComplete.cpp

Add test to sema

Would be cool if in the future we could instead transform something like

Thanks for the fix, LGTM! Do you want me to commit this for you?

Did a quick test on my machine, seems to work! Can you add a regression test to clang-tools-extra/clangd/unittests/tweaks/RemoveUsingNamespaceTests.cpp though?

The test in clangd/unittests/CodeCompleteTests.cpp still passes. We now probably also need a unit test for Sema that tests the new flag, right?

Move logic to SemaCodeComplete.cpp