[PATCH] D72746: [clangd] Add a flag for implicit references in the Index

kbobyrev updated this revision to Diff 242809. kbobyrev marked 2 inline comments as done. kbobyrev added a comment. Address another round of comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72746/new/ https://reviews.llvm.org/D72746 Files:

[PATCH] D72746: [clangd] Add a flag for implicit references in the Index

hokein accepted this revision. hokein added a comment. This revision is now accepted and ready to land. thanks, looks good, a few nits. before landing the patch, could you please run the clangd-indexer on the llvm project and measure the before/after indexing time? to make sure we don't

[PATCH] D72746: [clangd] Add a flag for implicit references in the Index

merge_guards_bot added a comment. {icon question-circle color=gray} Unit tests: unknown. {icon question-circle color=gray} clang-tidy: unknown. {icon question-circle color=gray} clang-format: unknown. Build artifacts :

[PATCH] D72746: [clangd] Add a flag for implicit references in the Index

kbobyrev updated this revision to Diff 242578. kbobyrev added a comment. Remove an artifact. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72746/new/ https://reviews.llvm.org/D72746 Files: clang-tools-extra/clangd/index/Ref.h

[PATCH] D72746: [clangd] Add a flag for implicit references in the Index

merge_guards_bot added a comment. {icon question-circle color=gray} Unit tests: unknown. {icon question-circle color=gray} clang-tidy: unknown. {icon question-circle color=gray} clang-format: unknown. Build artifacts :

[PATCH] D72746: [clangd] Add a flag for implicit references in the Index

kbobyrev updated this revision to Diff 242577. kbobyrev marked 7 inline comments as done. kbobyrev added a comment. Address the comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72746/new/ https://reviews.llvm.org/D72746 Files:

[PATCH] D72746: [clangd] Add a flag for implicit references in the Index

hokein added a comment. thanks, mostly good. the only concern from my side is about the `SymbolRole::Implicit`, I think we should get rid of it. Comment at: clang-tools-extra/clangd/index/Ref.h:53 + // The reference explicitly spells out declaration's name. Such references

[PATCH] D72746: [clangd] Add a flag for implicit references in the Index

merge_guards_bot added a comment. {icon times-circle color=red} Unit tests: fail. 61851 tests passed, 6 failed and 780 were skipped. failed: libc++.std/language_support/cmp/cmp_partialord/partialord.pass.cpp failed: libc++.std/language_support/cmp/cmp_strongeq/cmp.strongeq.pass.cpp

[PATCH] D72746: [clangd] Add a flag for implicit references in the Index

merge_guards_bot added a comment. {icon times-circle color=red} Unit tests: fail. 61851 tests passed, 6 failed and 780 were skipped. failed: libc++.std/language_support/cmp/cmp_partialord/partialord.pass.cpp failed: libc++.std/language_support/cmp/cmp_strongeq/cmp.strongeq.pass.cpp

[PATCH] D72746: [clangd] Add a flag for implicit references in the Index

kbobyrev updated this revision to Diff 241994. kbobyrev added a comment. Remove unnecessary include. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72746/new/ https://reviews.llvm.org/D72746 Files: clang-tools-extra/clangd/index/Ref.h

[PATCH] D72746: [clangd] Add a flag for implicit references in the Index

kbobyrev updated this revision to Diff 241993. kbobyrev marked 14 inline comments as done. kbobyrev added a comment. Address a number of comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72746/new/ https://reviews.llvm.org/D72746 Files:

[PATCH] D72746: [clangd] Add a flag for implicit references in the Index

kbobyrev added inline comments. Comment at: clang-tools-extra/clangd/index/Ref.h:32 + // + // class Foo {}; + // ^ Foo declaration hokein wrote: > could you confirm with it? this looks like a `Foo` class definition. Good point, I thought it should be

[PATCH] D72746: [clangd] Add a flag for implicit references in the Index

hokein added inline comments. Comment at: clang-tools-extra/clangd/index/Ref.h:32 + // + // class Foo {}; + // ^ Foo declaration could you confirm with it? this looks like a `Foo` class definition. Comment at:

[PATCH] D72746: [clangd] Add a flag for implicit references in the Index

merge_guards_bot added a comment. {icon check-circle color=green} Unit tests: pass. 61857 tests passed, 0 failed and 781 were skipped. {icon question-circle color=gray} clang-tidy: unknown. {icon check-circle color=green} clang-format: pass. Build artifacts

[PATCH] D72746: [clangd] Add a flag for implicit references in the Index

merge_guards_bot added a comment. {icon check-circle color=green} Unit tests: pass. 61857 tests passed, 0 failed and 781 were skipped. {icon question-circle color=gray} clang-tidy: unknown. {icon check-circle color=green} clang-format: pass. Build artifacts

[PATCH] D72746: [clangd] Add a flag for implicit references in the Index

kbobyrev updated this revision to Diff 239545. kbobyrev added a comment. Attempt to drop collected reference before doing more computation to improve performance. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72746/new/

[PATCH] D72746: [clangd] Add a flag for implicit references in the Index

kbobyrev added inline comments. Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:599 +const auto Tokens = FilesToTokensCache[MainFileID]; +for (auto : DeclRefs) { + if (auto ID = getSymbolID(DeclAndRef.first)) { hokein wrote: > note

[PATCH] D72746: [clangd] Add a flag for implicit references in the Index

kbobyrev updated this revision to Diff 239544. kbobyrev marked 7 inline comments as done. kbobyrev added a comment. Address review comments, add implicit references filter for SymbolCollector, test changes. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D72746: [clangd] Add a flag for implicit references in the Index

hokein added inline comments. Comment at: clang-tools-extra/clangd/index/Ref.h:34 + Spelled = 1 << 3, + All = Declaration | Definition | Reference | Spelled, }; kadircet wrote: > hokein wrote: > > The `All` now indicates all spelled refs. I think `All` should

[PATCH] D72746: [clangd] Add a flag for implicit references in the Index

kadircet added inline comments. Comment at: clang-tools-extra/clangd/index/Ref.h:34 + Spelled = 1 << 3, + All = Declaration | Definition | Reference | Spelled, }; hokein wrote: > The `All` now indicates all spelled refs. I think `All` should include both >

[PATCH] D72746: [clangd] Add a flag for implicit references in the Index

hokein added a comment. the scope of this patch is not very clear, the changes touch two different code parts `SymbolCollector`, and `Rename`, and we are lacking tests for `SymbolCollector`. I'd suggest spliting this patch into smaller patches: - a patch that adds a new kind to the ref, and

[PATCH] D72746: [clangd] Add a flag for implicit references in the Index

merge_guards_bot added a comment. {icon check-circle color=green} Unit tests: pass. 61855 tests passed, 0 failed and 781 were skipped. {icon question-circle color=gray} clang-tidy: unknown. {icon check-circle color=green} clang-format: pass. Build artifacts

[PATCH] D72746: [clangd] Add a flag for implicit references in the Index

merge_guards_bot added a comment. {icon check-circle color=green} Unit tests: pass. 61855 tests passed, 0 failed and 781 were skipped. {icon question-circle color=gray} clang-tidy: unknown. {icon check-circle color=green} clang-format: pass. Build artifacts

[PATCH] D72746: [clangd] Add a flag for implicit references in the Index

merge_guards_bot added a comment. {icon check-circle color=green} Unit tests: pass. 61855 tests passed, 0 failed and 781 were skipped. {icon question-circle color=gray} clang-tidy: unknown. {icon check-circle color=green} clang-format: pass. Build artifacts

[PATCH] D72746: [clangd] Add a flag for implicit references in the Index

kbobyrev updated this revision to Diff 239503. kbobyrev added a comment. Move added rename unit test to the end of the list. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72746/new/ https://reviews.llvm.org/D72746 Files:

[PATCH] D72746: [clangd] Add a flag for implicit references in the Index

kbobyrev updated this revision to Diff 239502. kbobyrev added a comment. Actually, this approach might be more generic (i.e. not relying on implementation too much). Added the FIXME. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72746/new/

[PATCH] D72746: [clangd] Add a flag for implicit references in the Index

kbobyrev planned changes to this revision. kbobyrev added a comment. Will put a TODO and revert helpers back to use plain binary search over the tokens. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72746/new/ https://reviews.llvm.org/D72746

[PATCH] D72746: [clangd] Add a flag for implicit references in the Index

kbobyrev updated this revision to Diff 239498. kbobyrev added a comment. Address review comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72746/new/ https://reviews.llvm.org/D72746 Files: clang-tools-extra/clangd/index/Ref.h

[PATCH] D72746: [clangd] Add a flag for implicit references in the Index

kadircet added a comment. In D72746#1827608 , @kbobyrev wrote: > 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

[PATCH] D72746: [clangd] Add a flag for implicit references in the Index

merge_guards_bot added a comment. {icon times-circle color=red} Unit tests: fail. 61674 tests passed, 181 failed and 781 were skipped. failed: Clangd.Clangd/background-index.test failed: Clangd.Clangd/code-action-request.test failed: Clangd.Clangd/compile-commands-path-in-initialize.test

[PATCH] D72746: [clangd] Add a flag for implicit references in the Index

kbobyrev updated this revision to Diff 238902. kbobyrev added a comment. 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,

[PATCH] D72746: [clangd] Add a flag for implicit references in the Index

merge_guards_bot added a comment. {icon check-circle color=green} Unit tests: pass. 61855 tests passed, 0 failed and 781 were skipped. {icon question-circle color=gray} clang-tidy: unknown. {icon check-circle color=green} clang-format: pass. Build artifacts

[PATCH] D72746: [clangd] Add a flag for implicit references in the Index

kbobyrev planned changes to this revision. kbobyrev added a comment. 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. Repository: rG LLVM Github Monorepo CHANGES SINCE

[PATCH] D72746: [clangd] Add a flag for implicit references in the Index

kbobyrev updated this revision to Diff 238786. kbobyrev marked 4 inline comments as done. kbobyrev added a comment. Address alsmost all comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72746/new/ https://reviews.llvm.org/D72746 Files:

[PATCH] D72746: [clangd] Add a flag for implicit references in the Index

kbobyrev added inline comments. Comment at: clang-tools-extra/clangd/index/Ref.h:36 + // one below. + Implicit = 1 << 3, + All = Declaration | Definition | Reference | Implicit, kadircet wrote: > kadircet wrote: > > instead of doing that, could we rather

[PATCH] D72746: [clangd] Add a flag for implicit references in the Index

hokein added a comment. In D72746#1821097 , @kbobyrev wrote: > 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`

[PATCH] D72746: [clangd] Add a flag for implicit references in the Index

kadircet added inline comments. Comment at: clang-tools-extra/clangd/index/Ref.h:36 + // one below. + Implicit = 1 << 3, + All = Declaration | Definition | Reference | Implicit, kadircet wrote: > instead of doing that, could we rather de-couple two enums

[PATCH] D72746: [clangd] Add a flag for implicit references in the Index

kadircet added inline comments. Comment at: clang-tools-extra/clangd/index/Ref.h:36 + // one below. + Implicit = 1 << 3, + All = Declaration | Definition | Reference | Implicit, instead of doing that, could we rather de-couple two enums completely and have a

[PATCH] D72746: [clangd] Add a flag for implicit references in the Index

merge_guards_bot added a comment. {icon check-circle color=green} Unit tests: pass. 61855 tests passed, 0 failed and 781 were skipped. {icon question-circle color=gray} clang-tidy: unknown. {icon check-circle color=green} clang-format: pass. Build artifacts

[PATCH] D72746: [clangd] Add a flag for implicit references in the Index

kbobyrev added a comment. 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

[PATCH] D72746: [clangd] Add a flag for implicit references in the Index

kbobyrev created this revision. kbobyrev added a reviewer: kadircet. Herald added subscribers: usaxena95, arphaman, jkorous, MaskRay, ilya-biryukov. Herald added a project: clang. kbobyrev added a subscriber: hokein. This patch allows the index does to provide a way to distinguish implicit