I started using TokenBuffer, but I ran into the following issue: when I'm
creating TokenBuffer and TokenCollector, they do not contain any tokens.
Preprocessor does not seem to have a non-null Lexer instance, TokenWatcher
(set in TokenCollector) is not triggered anywhere and neither is
Lexer::Lex. I don't have much familiarity with the code and I looked at the
other usages of TokenBuffer but didn't figure out what's wrong with the code
in this patch. I suspect the lexer in Preprocessor should be re-initialized
somehow? I'm certainly missing something here.

Address comments

Need to make use of the TokenBuffer, ran into some difficulties when lexing some files and triggering assertions in the process. Will figure it out and update the patch.

Address alsmost all comments.

Remove duplicated testt

The patch should probably be correct now.

Avoid duplicate references by filtering out destructor calls

Going to investigate few interesting cases of findExplicitReferences not working as intended and opting out for a clean approach that prevents duplicates from getting into the reference set.

The patch could be shorter and slightly less confusing if I preserved 1:1 RefKind::Implicit <-> index::SymbolKind::Implicit relation, but I thought we might want to keep RefKind being 1 byte so that we don't waste unnecessary memory. Also, since we might want to start using findExplicitReferences (or something else we come up with) for indexing, it shouldn't be critical. Let me know if you think it's unnecessary.

Changed reviewer to @kadircet because Sam would be out until the end of the week.

I'm not sure if leaving both ReferenceLocs pointing to the same location is a sensible solution, but merging them seems quite complicated and probably not really worth the effort. I've considered multiple alternative solutions, as described in https://github.com/clangd/clangd/issues/236 and converged to the one presented in this patch.

Improve wording in the comment

@sammccall Indexed.size() > Lexed.size() is one of the assumptions that I think might not hold in real-world scenarios. I was reading patch heuristics code a lot and trying to understand whether anything breaks when this check is not in place, but I couldn't come up with some cases, so I thought it'd be OK to remove it.

For some reason, running this patch on a recent version of the source tree fails. The AST parsing fails with fatal diagnostics, but I can't understand why:

This is a WIP draft. Few items to address before pushing for a review

LGTM

Addressed a bunch of comments to cleanup the patch and replied to ask for clarification of several unresolved comments.

Move helper function back to the anonymous namespace.

Sorry for a delay: I was trying to work with range patching heuristics and get it to work in generic case of "stale index returns more results than lexer", but in the end I converged to the simplest possible version of the intended change.

(also, this should eliminate unwanted changes caused by the index being stale, wouldn't it?)

In D71598#1787806, @hokein wrote:

(apologies, the FIXME may imply this approach...)

this approach is based on an assumption: the index results are matched to the latest file content, but this is not always true in practice, our index maybe stale (index results came from an old snapshot of the file), then this approach will fail.

I think we should do it in another direction:

• add a new RefKind (something like implicit references, or named references) to clangd::Ref
• when querying the index for rename, we set a corresponding Filter in the query request (or filter out non-interesting references based on the RefKind aftjerwards)

In D71455#1787266, @MaskRay wrote:

Renaming handleDeclOccurence, handleMacroOccurence and handleModuleOccurence are definitely not NFC because they are public APIs that can be used by downstream projects.

Rebase on top of master, fix the build and apply formatting.

Address the comments and simplify the code.

Need to address the comments.

Add another test for ~^Foo and rebase on top of master.

Improve wording in the comment I moved.

In D71029#1769416, @sammccall wrote:

@kbobyrev would you mind verifying this version of the patch also works?
I don't expect changes vs the last one: it's been restructured a lot but the logic should be the same.

In D70863#1764977, @sammccall wrote:

@kbobyrev would you mind testing this again?

Dropping an empty compile_flags.txt should also work (and test the other codepath)

Probably not worth investigating at this point.

The current version of the patch fixes one of the related issues. When running on a simple file like this

Sorry, seems I was too late :(

Rebase on top of master.

Get rid of incomplete identifiers in test code.

Simplified the patch to address the comments.

Add initial support for generic lambdas, put more tests into clang/test/CodeCompletion.

In D69295#1719069, @terrelln wrote:

I also agree with this. I think llvm/benchmarks would be fine for now either way, but with more benchmarks (which hopefully would be there at some point, I had plans of adding some for a while now but I'm not sure when I'll get to that) subdirectories make sense.

I agree it would be better in llvm/benchmarks, and that is what I tried that first, but had problems with getting the CMakeFile.txt right. I set up the file like this:

set(LLVM_LINK_COMPONENTS
  Support)

add_benchmark(DummyYAML DummyYAML.cpp)
add_benchmark(hash-benchmark hash-benchmark.cpp)

But the first benchmark complained that hash-benchmark.cpp is unused, and the second complained that DummyYAML.cpp is unused in LLVMProcessSources.cmake:112. If someone can help me with the CMake, I'd be glad to switch it back.

Ah, good point! I'll look into that, thank you for bringing it up.

Is it a -DCMAKE_BUILD_TYPE=Release build? I will be pretty surprised if gcc/clang -O3 does not inline these functions.

Yeah, clang -O3 inlines all of them, but gcc -O3 inlines most, but not all of them before this patch. gcc-6 doesn't inline at least some calls of r1, blk, and r3.

In D69295#1718448, @MaskRay wrote:

Add inline to the helper functions because gcc-9 won't inline all of them without the hint. I've avoided attribute((always_inline)) because gcc and clang will inline without it, and improves compatibility.

Is it a -DCMAKE_BUILD_TYPE=Release build? I will be pretty surprised if gcc/clang -O3 does not inline these functions.

The code change looks good, except that the llvm/benchmarks/ directory is underused and you are adding more files.

Yes, I think putting files into llvm/benchmarks would be sensible especially given the lack of other benchmarks right now.

We probably need more consensus (e.g. send an email to the llvm-dev mailing list, referencing http://lists.llvm.org/pipermail/llvm-dev/2018-August/125173.html @kbobyrev) on how benchmark files should be organized.

I also agree with this. I think llvm/benchmarks would be fine for now either way, but with more benchmarks (which hopefully would be there at some point, I had plans of adding some for a while now but I'm not sure when I'll get to that) subdirectories make sense.

The change looks good to me, thank you for the patch!

Could you please elaborate a bit on why you would like to propose this change? I couldn't understand the argument here:

Remove empty Clang SA section for now.

@aprantl if there is still some work left for GSoC/potential contributors, feel free to mention it and then it should be just updated for the future. I couldn't infer whether there is still a chunk of work to be done from the project report, but you might have more information.