Mostly NITs from my side, the change LG, thanks!

We seem to have trouble only in presence of using declarations and using namespace directives.
I wonder how complicated it would be to take them into account instead. That would clearly be easier to read, as we'll hit right into the center of the problem.

D66647 was updated to use findExplicitReferences, abandoning this revision.

This was finalized and landed as D68562

An alternative approach I'm thinking of:
After D68977 lands, we could try using findExplicitReferences to produce all of these edits:

1. we collect locations of all references and declaration of relevant parameters and template parameters.
2. find the ones where the name differs and produce the changes.

Mostly LG, but please take a look at the NITs and the implementation-related comments.

LGTM from my side, but not marking as accepted yet to make sure @hokein has a chance to take a final look.

In D67901#1704639, @nridge wrote:

I like how we went from using heuristics, to not using heuristics, to using heuristics again :)

But admittedly, the new heuristics are more accurate because they're based on phase 1 lookup results rather than just syntax, so I'm happy with the outcome!

LGTM, many thanks!

Many thanks, this LG overall, just a few NITs and documentation requests.

Just noticed the next version of LSP added diagnostic tags for things like "unused field" or "dead code":
https://microsoft.github.io/language-server-protocol/specifications/specification-3-15, search for DiagnosticTag.

In D64799#1688293, @dgoldman wrote:

In D64799#1605514, @ilya-biryukov wrote:

Tried the suggested approach and ran into the issue described above.
Either we make the diagnostics less useful ('did you mean foo::bar?' turns into 'unresolved identifier bar'; without mentioning the correction) or we even stop providing some corrections in addition to that.

On the other hand, I agree that over time we will start emitting too many diagnostics at the end of TU if keep the patch as is. That is not a good way.
There should be a better option for emitting the uncorrected diagnostics closer to where they are produced, but I don't have a good idea on what it should be off the top of my head. Ideas warmly welcome.

I might be wrong here, but I thought that diagnostics were delayed until typo correction has been completed on an expression. For example, you'll only get something like unresolved identifier bar instead of did you mean foo::bar? only when you call the DiagHandler with either a proper TypoCorrection or an empty one. If so, I'm not sure how you'd get into this case if you're calling CorrectDelayedTyposInExpr and tracking which typos have been resolved already.

In D67536#1700872, @nridge wrote:

It also lets them consistently highlight part of the line (e.g. dead expressions or statements can be marked in gray even if they are on the same line).

Highlighting part of a line is not applicable to inactive preprocessor branches in C++.

Note that the opposite is true - inactive preprocessor branches can be expressed as range-based highlightings.

In D68630#1699371, @kuhnel wrote:

as the automatic build report did not work:

In D67536#1697825, @sammccall wrote:

So I don't think clients will/should prefer that - for best rendering they should know this is a line highlight.

Thanks!

Could you add a rationale in the change description? E.g. something like we do not have any tests for findNextToken

The main comment is about limiting this only to global namespace. PTAL.

LGTM

LGTM

LGTM

• Address comments

It just hit me that we should probably just wait for the async request to finish before returning from completion.
We might be doing this to reduce latency a little, but I don't recall us measuring that this provides a significant performance win.

This is still racy if there were > 1 request. I wonder if there a better way to address this?

Many thanks! LGTM

In D66647#1686021, @kadircet wrote:

So currently AST doesn't store any information regarding template parameter locations except the deepest one.
Therefore I've changed the availability to discard any methods inside templated classes, since there is no way to
validate the template parameter names. Hopefully this should be a rare use-case, and I believe most of the times
people have same template paramater names on declaration and definition. Let me know what you think about
it.

• Restore dlog()
• Check reference to function pointer non-type template parameters in tests
• Extend comment of TraverseTemplateArgumentLoc

Mostly LGTM

Mostly NITs, except the naming of the new TokenKind enum.
I think it's better to pick something that's not clashing with clang::tok::TokenKind, even if the enum is in a different namespace.

• Add a comment

• Add tests for non-type template paremeters

The patch breaks some tests, see
http://lab.llvm.org:8011/builders/clang-cmake-x86_64-sde-avx512-linux/builds/27891/steps/ninja%20check%201/logs/FAIL%3A%20Clang-Unit%3A%3AFormatTest.FunctionAnnotations

In D66647#1685506, @kadircet wrote:

I actually have a fixme saying:

// FIXME: Instead of fully qualifying we should try deducing visible scopes at
// target location and generate minimal edits.

Elaborating on it by saying "we can start by using the namespaces in targetcontext".

In D68120#1685409, @kadircet wrote:

LGTM. can you also add some tests for template template and non-type template params ?

LGTM

We also need to rename parameters sometimes, right?

// Sometimes we need to rename parameters.
void usages(int decl_param, int);

Currently we add too many qualifiers in some cases, e.g. here's a common pattern in clangd:

// foo.h
#include "Decl.h"
namespace clang::clangd { std::string printName(Decl*D); }

LGTM

LGTM

A few go-by comments from me, Haojian should have better context here!

@kadircet says we're using this pattern all over clangd. This somehow slipped my view before.
LGTM to be consistent with the rest of clangd (and functions in FindTarget.h).

Who are the callers? Selection tree?
FWIW, I'd rather pass non-const everywhere, the concept of const-ness is just not very useful for AST nodes.

• s/Decl* S/Decl* D/g

LGTM

I believe all comments were addressed. PTAL and let me know if I missed something.