@sammccall, how would you feel about proceeding with the patch in its current state, with the memory usage increase brought down from 8.2% to 2.5% thanks to the combination of the simple lookup optimization + RefKind filtering, and leaving the "deep lookup optimization" to be explored in a future change?

Address other review comments

The updated patch additionally implements the "simple lookup optimization" discussed in the review.

Implement the "simple lookup optimization"

The updated patch implements one of the optimizations discussed during review, namely filtering the Refs stored in the RevRefs data structure to just those that could be calls.

Implement optimization to filter the refs stored in RevRefs to calls only

To be able to reason about the impact of various memory usage optimizations, I took some baseline measurements.

In D93829#4422090, @qchateau wrote:

So if anyone wants to take it from there, feel free to reuse my commits (or not).

Rebased to apply to recent trunk

In D145843#4221826, @nridge wrote:

Thanks for the clarification and suggested formulation.

In D145843#4209750, @sammccall wrote:

I'd suggest something like:

Style:
  QuotedHeaders: "path/to/.*"
  AngledHeaders: "path/sdk/.*"

where the regexes can be single or array, and match any suffix (like Diagnostics.Includes.IgnoreHeader)

@ADKaster are you interested in updating the patch to implement this approach?

Thank you, the updates look good! Please go ahead and merge after addressing the last minor nits.

This can be closed, as the change has been made in https://github.com/llvm/llvm-project/pull/65824

I'm fine with allowing partial selection for consistency with other expressions.

This was superseded by D146941.

I'm not well versed in the parser code, but the fix clearly avoids the assertion in getIdentifierInfo(), and both @aaron.ballman and @rjmccall indicated in the issue discussion that this fix is appropriate, so I will take the liberty of approving this.

(Otherwise the updates look good!)

In D153114#4630318, @ChuanqiXu wrote:

However, I can't search the caller of reparseOpenFilesIfNeeded which semantics matches the behavior. The two callers of reparseOpenFilesIfNeeded I found are ClangdLSPServer::applyConfiguration() and ClangdLSPServer::onDocumentDidSave() and neither of them matches description files included in the preamble have been modified.

So I want to ask what's the behavior when I update a header and where is the corresponding code. Thanks.

(I'm away on travels, will get back to this within the next week)

Address review comment

Committed, thanks!

In D153114#4603579, @sammccall wrote:

Dep scanning - roles

IIUC we do this for two reasons:

• to identify what module names we must have PCMs for in order to build a given TU (either an open file, or a module we're building as PCM)
• to build a database mapping module name => filename, which we compose with the CDB to know how to build a PCM for a given module name

I think it would be good to clearly separate these. The latter is simpler, more performance-critical, async, and is probably not used at all if the build system can tell us this mapping.
The latter is more complex, and will always be needed synchronously for the mainfile regardless of the build system.

Thanks for the patch!

A future enhancement to consider could be, in the following testcase:

Address review comments

Address review comment

Ok, I've finished looking through the patch; sorry it took so long to get to.

Fix naming nit

Rebase and address review comment

Address review comment

Adding Sam, since you're on a review roll ;)

I understood https://github.com/clangd/clangd/issues/1344 as being about workspace/symbol rather than textDocument/documentSymbol, though I see now that both are affected.

Haven't looked at everything yet, but wanted to mention one thing I noticed.

Thanks for the patch, these are nice improvements

I promise I haven't forgotten about this. It's just been a challenge finding a large enough chunk of time to page in all the relevant context and think through the issues. Maybe this weekend...

In D155370#4550042, @zyounan wrote:

in the context where CanBeCall=false, parameters don't disambiguate against the overloads, so we'd end up with the same function name after selecting the candidate

Thanks!

LGTM

I'm not sure how I feel about dropping the parameters from the signature in the CanBeCall = false case.

Your patience is appreciated. I have a number of patches in my review queue, and 3 days is often not a realistic turnaround time for me.

In D150124#4542032, @kadircet wrote:

btw, thanks a lot Nathan for taking good care of clangd, I don't think I say that enough

Noticed this while reviewing D150124. No caller was providing a value for this parameter.

Adding Sam as well in case he has any thoughts on the discussion (another user ran into this recently and filed https://github.com/clangd/clangd/issues/1694)

In D150124#4329163, @ilya-biryukov wrote:

In clangd we also have findExplicitReferences and targetDecl functions that seem to handle the GoToStmt.
In fact, I believe they are used in findDocumentHighlights, so I'm not sure why your test did not work before this patch.
findDocumentHighlights

Abandoning in favour of D155381.

Add testcase

Yeah, I'm happy to go with D155381 instead.

Thanks! I agree that this no-configuration approach is nicer.

In D140745#4505829, @sammccall wrote:

ping on this one for when you have time

Also adding links to the open issues this fixes:

Let's try adding some reviewers

I'm sorry, I feel like I don't have a good enough level of insight into the requirements to usefully critique this patch, nor the bandwidth to take a detailed look through the implementation right now.

Thank you for the suggested testcase!

Looks reasonable to me but Sam should OK this.

Requesting review.