[PATCH] D42640: [clangd] collect symbol #include & insert #include in global code completion.

ioeric updated this revision to Diff 134394. ioeric added a comment. - Merged with origin/master Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D42640 Files: clangd/CMakeLists.txt clangd/ClangdLSPServer.cpp clangd/ClangdServer.cpp clangd/ClangdServer.h

[PATCH] D43429: [clangd] Add missing library (clangLex) in a few places

ioeric accepted this revision. ioeric added a comment. This revision is now accepted and ready to land. lg. Thanks! Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D43429 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D43550: [clangd] Not collect include headers for dynamic index for now.

ioeric created this revision. ioeric added reviewers: ilya-biryukov, sammccall, hokein. Herald added subscribers: cfe-commits, jkorous-apple, klimek. The new behaviors introduced by this patch: o When include collection is enabled, we always set IncludeHeader field in Symbol even if it's the

[PATCH] D43550: [clangd] Not collect include headers for dynamic index for now.

ioeric added a comment. In https://reviews.llvm.org/D43550#1014319, @ilya-biryukov wrote: > Is there a way to still enable include insertion but in a restricted manner, > e.g. only for the current project? I'm not sure if this is currently supported, as we don't have a good definition of a

[PATCH] D43567: [ASTMatchers] isTemplateInstantiation: also match explicit instantiation declaration.

This revision was automatically updated to reflect the committed changes. Closed by commit rL325678: [ASTMatchers] isTemplateInstantiation: also match explicit instantiation… (authored by ioeric, committed by ). Herald added a subscriber: llvm-commits. Repository: rL LLVM

[PATCH] D43550: [clangd] Not collect include headers for dynamic index for now.

ioeric added inline comments. Comment at: clangd/index/FileIndex.cpp:18 -const CanonicalIncludes *canonicalIncludesForSystemHeaders() { - static const auto *Includes = [] { ilya-biryukov wrote: > Should we also remove the mutex from `CanonicalIncludes` now? I

[PATCH] D43550: [clangd] Not collect include headers for dynamic index for now.

ioeric updated this revision to Diff 135244. ioeric marked 2 inline comments as done. ioeric added a comment. Properly use toURI. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D43550 Files: clangd/CodeComplete.cpp clangd/index/FileIndex.cpp clangd/index/Index.h

[PATCH] D43550: [clangd] Not collect include headers for dynamic index for now.

This revision was automatically updated to reflect the committed changes. Closed by commit rL325764: [clangd] Not collect include headers for dynamic index for now. (authored by ioeric, committed by ). Herald added a subscriber: llvm-commits. Repository: rL LLVM

[PATCH] D43462: [clangd] Fixes for #include insertion.

ioeric created this revision. ioeric added reviewers: sammccall, ilya-biryukov. Herald added subscribers: cfe-commits, jkorous-apple, klimek. o Avoid inserting a header include into the header itself. o Avoid inserting non-header files. o Canonicalize include paths for symbols in dynamic index.

[PATCH] D43510: [clangd] don't insert new includes if either original header or canonical header is already included.

ioeric updated this revision to Diff 135074. ioeric marked 16 inline comments as done. ioeric added a comment. - Addressed review comments. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D43510 Files: clangd/ClangdLSPServer.cpp clangd/ClangdServer.cpp

[PATCH] D43510: [clangd] don't insert new includes if either original header or canonical header is already included.

ioeric added inline comments. Comment at: clangd/ClangdServer.h:243 + /// string quoted with <> or "" that can be #included directly. + /// \p PreferredHeader The preferred header to be inserted. The has the same + /// semantic as \p OriginalHeader. If empty, \p

[PATCH] D43462: [clangd] Fixes for #include insertion.

ioeric updated this revision to Diff 134932. ioeric marked 6 inline comments as done. ioeric added a comment. - Stop indexing main files in dynamic index; addressed review comments. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D43462 Files: clangd/ClangdServer.cpp

[PATCH] D43462: [clangd] Fixes for #include insertion.

ioeric added inline comments. Comment at: clangd/Headers.cpp:45 +bool hasHeaderExtension(PathRef Path) { + constexpr static const char *HeaderExtensions[] = {".h", ".hpp", ".hh", + ".hxx"}; ilya-biryukov

[PATCH] D43634: [clangd] Extend textDocument/didChange to specify whether diagnostics should be generated.

This revision was automatically updated to reflect the committed changes. Closed by commit rCTE325813: [clangd] Extend textDocument/didChange to specify whether diagnostics should be… (authored by ioeric, committed by ). Changed prior to commit:

[PATCH] D43500: [clang-tidy]: modernize-use-default-member-init: Remove trailing comma and colon.

ioeric added a comment. In https://reviews.llvm.org/D43500#1015208, @jdemeule wrote: > In https://reviews.llvm.org/D43500#1013558, @malcolm.parsons wrote: > > > In https://reviews.llvm.org/D43500#1013497, @aaron.ballman wrote: > > > > > Is there a way to make clang-apply-replacements smarter

[PATCH] D43634: [clangd] Extend textDocument/didChange to specify whether diagnostics should be generated.

ioeric updated this revision to Diff 135463. ioeric marked 2 inline comments as done. ioeric added a comment. Addressed review comments. Removed test. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D43634 Files: clangd/ClangdLSPServer.cpp clangd/Protocol.cpp

[PATCH] D43634: [clangd] Extend textDocument/didChange to specify whether diagnostics should be generated.

ioeric created this revision. ioeric added a reviewer: sammccall. Herald added subscribers: cfe-commits, jkorous-apple, ilya-biryukov, klimek. This would allow us to disable diagnostics when didChange is called but diagnostics are not wanted (e.g. code completion). Repository: rCTE Clang

[PATCH] D43671: [clangd] Address FIXME and fix comment

ioeric accepted this revision. ioeric added a comment. This revision is now accepted and ready to land. Oops, just realized I forgot to push the "send" button! Comment at: clangd/tool/ClangdMain.cpp:153 + if (RunSynchronously) { +if

[PATCH] D42640: [clangd] collect symbol #include & insert #include in global code completion.

This revision was automatically updated to reflect the committed changes. Closed by commit rCTE325343: [clangd] collect symbol #include insert #include in global code completion. (authored by ioeric, committed by ). Changed prior to commit: https://reviews.llvm.org/D42640?vs=134600=134603#toc

[PATCH] D42640: [clangd] collect symbol #include & insert #include in global code completion.

ioeric updated this revision to Diff 134600. ioeric marked 3 inline comments as done. ioeric added a comment. - Merged with origin/master - Addressed review comments; removed .inc handling. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D42640 Files: clangd/CMakeLists.txt

[PATCH] D42640: [clangd] collect symbol #include & insert #include in global code completion.

ioeric added a comment. Thank you for reviewing this! Comment at: clangd/index/CanonicalIncludes.h:52 + /// a canonical header name. + llvm::StringRef mapHeader(llvm::StringRef Header) const; + sammccall wrote: > ioeric wrote: > > sammccall wrote: > > > So

[PATCH] D42640: [clangd] collect symbol #include & insert #include in global code completion.

This revision was automatically updated to reflect the committed changes. Closed by commit rL325343: [clangd] collect symbol #include insert #include in global code completion. (authored by ioeric, committed by ). Herald added a subscriber: llvm-commits. Repository: rL LLVM

[PATCH] D43567: [ASTMatchers] isTemplateInstantiation: also match explicit instantiation declaration.

ioeric created this revision. ioeric added a reviewer: bkramer. Herald added subscribers: cfe-commits, klimek. Example: template class X {}; class A {}; // Explicit instantiation declaration. extern template class X; Repository: rC Clang https://reviews.llvm.org/D43567 Files:

[PATCH] D43510: [clangd] don't insert new includes if either original header or canonical header is already included.

ioeric created this revision. ioeric added a reviewer: sammccall. Herald added subscribers: cfe-commits, jkorous-apple, ilya-biryukov, klimek. Changes: o Store both the original header and the canonical header in LSP command. o Also check that both original and canonical headers are not already

[PATCH] D43437: Fix link failures for Preprocessor::addCommentHandler

ioeric accepted this revision. ioeric added a subscriber: malaperle. ioeric added a comment. This revision is now accepted and ready to land. lg. Thanks! fyi, @malaperle also sent https://reviews.llvm.org/D43429 for the same fix but has not landed it yet ;) Repository: rCTE Clang Tools

[PATCH] D43462: [clangd] Fixes for #include insertion.

ioeric updated this revision to Diff 134944. ioeric marked 2 inline comments as done. ioeric added a comment. - added a comment about thread safety Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D43462 Files: clangd/ClangdServer.cpp clangd/Headers.cpp clangd/Headers.h

[PATCH] D43462: [clangd] Fixes for #include insertion.

ioeric updated this revision to Diff 134943. ioeric marked 3 inline comments as done. ioeric added a comment. - addressed comments. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D43462 Files: clangd/ClangdServer.cpp clangd/Headers.cpp clangd/Headers.h

[PATCH] D43462: [clangd] Fixes for #include insertion.

ioeric marked 5 inline comments as done. ioeric added inline comments. Comment at: clangd/Headers.cpp:60 Argv.push_back(S.c_str()); IgnoringDiagConsumer IgnoreDiags; auto CI = clang::createInvocationFromCommandLine( ilya-biryukov wrote: > Not related

[PATCH] D43462: [clangd] Fixes for #include insertion.

This revision was automatically updated to reflect the committed changes. Closed by commit rCTE325523: [clangd] Fixes for #include insertion. (authored by ioeric, committed by ). Changed prior to commit: https://reviews.llvm.org/D43462?vs=134947=134952#toc Repository: rL LLVM

[PATCH] D43462: [clangd] Fixes for #include insertion.

This revision was automatically updated to reflect the committed changes. Closed by commit rL325523: [clangd] Fixes for #include insertion. (authored by ioeric, committed by ). Herald added a subscriber: llvm-commits. Repository: rL LLVM https://reviews.llvm.org/D43462 Files:

[PATCH] D43462: [clangd] Fixes for #include insertion.

ioeric updated this revision to Diff 134947. ioeric added a comment. - assert in the very beginning! Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D43462 Files: clangd/ClangdServer.cpp clangd/Headers.cpp clangd/Headers.h clangd/index/CanonicalIncludes.cpp

[PATCH] D43510: [clangd] don't insert new includes if either original header or canonical header is already included.

ioeric updated this revision to Diff 135857. ioeric marked an inline comment as done. ioeric added a comment. - Merge with origin/master - Use llvm::StringSet Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D43510 Files: clangd/ClangdLSPServer.cpp clangd/ClangdServer.cpp

[PATCH] D43510: [clangd] don't insert new includes if either original header or canonical header is already included.

This revision was automatically updated to reflect the committed changes. Closed by commit rL326070: [clangd] dont insert new includes if either original header or canonical… (authored by ioeric, committed by ). Herald added a subscriber: llvm-commits. Repository: rL LLVM

[PATCH] D43671: [clangd] Address FIXME and fix comment

ioeric added a comment. Thanks for the cleanup Kirill :) Comment at: clangd/tool/ClangdMain.cpp:153 + if (RunSynchronously) { +if (WorkerThreadsCount != 0) { + llvm::errs() `-j` is non-zero by default, and we shouldn't show warning if users only

[PATCH] D41668: [clangd] Add static index for the global code completion.

ioeric added inline comments. Comment at: clangd/ClangdLSPServer.h:40 + bool BuildDynamicSymbolIndex, + std::unique_ptr GlobalIdx); We are calling this global index and static index in the patch. I think we should be

[PATCH] D41668: [clangd] Add static index for the global code completion.

ioeric added a comment. You have mentioned that YAML data source is experimental only in the patch summary, but we should also mention this in the code. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D41668 ___ cfe-commits mailing

[PATCH] D41345: [clangd] Add more symbol information for code completion.

ioeric updated this revision to Diff 128505. ioeric added a comment. - Merge with origin/master. Use Arena for symbol details. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D41345 Files: clangd/ClangdUnit.cpp clangd/ClangdUnit.h clangd/CodeComplete.cpp

[PATCH] D41345: [clangd] Add more symbol information for code completion.

ioeric added inline comments. Comment at: clangd/index/Index.h:122 + + llvm::Optional Detail; + sammccall wrote: > ioeric wrote: > > sammccall wrote: > > > I think you probably want a raw pointer rather than optional: > > > - reduce the size of the struct when

[PATCH] D41668: [clangd] Add static index for the global code completion.

ioeric added inline comments. Comment at: clangd/CodeComplete.cpp:583 - Items->isIncomplete = !Index.fuzzyFind(Ctx, Req, [&](const Symbol ) { -Items->items.push_back(indexCompletionItem(Sym, Filter, SSInfo)); - }); + // FIXME: figure out a way to merge the symbols from

[PATCH] D41345: [clangd] Add more symbol information for code completion.

ioeric updated this revision to Diff 127781. ioeric marked an inline comment as done. ioeric added a comment. - Merged with origin/master - Addressed some more comments. - Add new fields to YAML. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D41345 Files:

[PATCH] D41345: [clangd] Add more symbol information for code completion.

ioeric added inline comments. Comment at: clangd/index/Index.h:105 + /// What to insert when completing this symbol (plain text version). + std::string CompletionPlainInsertText; + /// What to insert when completing this symbol (snippet version). sammccall

[PATCH] D48418: [clangd] Expose 'shouldCollectSymbol' helper from SymbolCollector.

This revision was automatically updated to reflect the committed changes. Closed by commit rL335218: [clangd] Expose shouldCollectSymbol helper from SymbolCollector. (authored by ioeric, committed by ). Herald added a subscriber: llvm-commits. Changed prior to commit:

[PATCH] D48425: [clangd] Expose qualified symbol names in CompletionItem (C++ structure only, no json).

ioeric created this revision. ioeric added a reviewer: sammccall. Herald added subscribers: cfe-commits, jkorous, MaskRay, ilya-biryukov. The qualified name can be used to match a completion item to its corresponding symbol. This can be useful for tools that measure code completion quality.

[PATCH] D48425: [clangd] Expose qualified symbol names in CompletionItem (C++ structure only, no json).

ioeric updated this revision to Diff 152265. ioeric added a comment. - Fix build Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D48425 Files: clangd/CodeComplete.cpp clangd/Protocol.h unittests/clangd/CodeCompleteTests.cpp Index: unittests/clangd/CodeCompleteTests.cpp

[PATCH] D48418: [clangd] Expose 'shouldCollectSymbol' helper from SymbolCollector.

ioeric updated this revision to Diff 152249. ioeric marked an inline comment as done. ioeric added a comment. - added tests. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D48418 Files: clangd/index/SymbolCollector.cpp clangd/index/SymbolCollector.h

[PATCH] D48418: [clangd] Expose 'shouldCollectSymbol' helper from SymbolCollector.

ioeric added a comment. In https://reviews.llvm.org/D48418#1138984, @sammccall wrote: > Can you motivate this change a bit, and add tests? Done. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D48418 ___ cfe-commits mailing list

[PATCH] D48425: [clangd] Expose qualified symbol names in CompletionItem (C++ structure only, no json).

ioeric updated this revision to Diff 152331. ioeric marked 2 inline comments as done. ioeric added a comment. - store scope in completion item. Add splitQualifiedName for NamedDecl. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D48425 Files: clangd/CodeComplete.cpp

[PATCH] D48425: [clangd] Expose qualified symbol names in CompletionItem (C++ structure only, no json).

ioeric added inline comments. Comment at: clangd/Protocol.h:743 + /// FIXME: find a more precise way to identify a completion item. + std::string QualifiedSymbolName; }; sammccall wrote: > So this is always set to scope + filterText (except for non-decl

[PATCH] D48290: [clangd] Use workspace root path as hint path for resolving URIs in workspace/symbol

This revision was automatically updated to reflect the committed changes. Closed by commit rL335035: [clangd] Use workspace root path as hint path for resolving URIs in… (authored by ioeric, committed by ). Herald added a subscriber: llvm-commits. Repository: rL LLVM

[PATCH] D48290: [clangd] Use workspace root path as hint path for resolving URIs in workspace/symbol

ioeric updated this revision to Diff 151869. ioeric marked 3 inline comments as done. ioeric added a comment. - Address review comments Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D48290 Files: clangd/ClangdServer.cpp clangd/FindSymbols.cpp clangd/FindSymbols.h

[PATCH] D48290: [clangd] Use workspace root path as hint path for resolving URIs in workspace/symbol

ioeric added inline comments. Comment at: clangd/ClangdServer.cpp:119 + auto FS = FSProvider.getFileSystem(); + auto Status = FS->status(RootPath); + if (!Status) sammccall wrote: > why validate it? This is the current behavior, except

[PATCH] D48290: [clangd] Use workspace root path as hint path for resolving URIs in workspace/symbol

ioeric updated this revision to Diff 151877. ioeric added a comment. - Require '/' in front of unittest: body Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D48290 Files: clangd/ClangdServer.cpp clangd/FindSymbols.cpp clangd/FindSymbols.h

[PATCH] D48441: [clangd] Incorporate transitive #includes into code complete proximity scoring.

ioeric added a comment. Looks great! Thanks for doing this! The algorithm looks pretty efficient. It might still make sense to collect some performance numbers for real files with potentially large #include tree (we have plenty of those in our internal codebase :) Comment

[PATCH] D48425: [clangd] Expose qualified symbol names in CompletionItem (C++ structure only, no json).

This revision was automatically updated to reflect the committed changes. Closed by commit rCTE335334: [clangd] Expose qualified symbol names in CompletionItem (C++ structure only… (authored by ioeric, committed by ). Changed prior to commit:

[PATCH] D48425: [clangd] Expose qualified symbol names in CompletionItem (C++ structure only, no json).

ioeric updated this revision to Diff 152447. ioeric marked 2 inline comments as done. ioeric added a comment. - Add printQualifiedName in AST.h Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D48425 Files: clangd/AST.cpp clangd/AST.h clangd/CodeComplete.cpp

[PATCH] D50189: Fully qualify the renamed symbol if the shortened name is ambiguous.

ioeric updated this revision to Diff 158761. ioeric added a comment. - minor update to comment. Repository: rC Clang https://reviews.llvm.org/D50189 Files: lib/Tooling/Core/Lookup.cpp unittests/Tooling/LookupTest.cpp Index: unittests/Tooling/LookupTest.cpp

[PATCH] D50189: Fully qualify the renamed symbol if the shortened name is ambiguous.

ioeric created this revision. ioeric added reviewers: ilya-biryukov, hokein. Herald added a subscriber: cfe-commits. For example, when renaming `a::b::x::foo` to `y::foo` below, replacing `x::foo()` with `y::foo()` can cause ambiguity. In such cases, we simply fully qualify the name with leading

[PATCH] D50189: Fully qualify the renamed symbol if the shortened name is ambiguous.

This revision was automatically updated to reflect the committed changes. Closed by commit rL338832: Fully qualify the renamed symbol if the shortened name is ambiguous. (authored by ioeric, committed by ). Herald added a subscriber: llvm-commits. Repository: rL LLVM

[PATCH] D50189: Fully qualify the renamed symbol if the shortened name is ambiguous.

ioeric added inline comments. Comment at: lib/Tooling/Core/Lookup.cpp:139 +const NamespaceDecl *NS = *I; +auto LookupRes = NS->lookup(DeclarationName((Head))); +if (!LookupRes.empty()) { ilya-biryukov wrote: > This will not take using namespaces into

[PATCH] D50189: Fully qualify the renamed symbol if the shortened name is ambiguous.

ioeric updated this revision to Diff 158941. ioeric marked 2 inline comments as done. ioeric added a comment. - address review comments. Repository: rC Clang https://reviews.llvm.org/D50189 Files: lib/Tooling/Core/Lookup.cpp unittests/Tooling/LookupTest.cpp Index:

[PATCH] D50154: [clangd] capitalize diagnostic messages if '-capitialize-diagnostic-text' is provided

ioeric added inline comments. Comment at: test/clangd/capitalize-diagnostics.test:1 +# RUN: clangd -lit-test < %s | FileCheck -strict-whitespace %s +{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}}

[PATCH] D49546: [clangd] Proof-of-concept query iterators for Dex symbol index

ioeric accepted this revision. ioeric added a comment. This revision is now accepted and ready to land. lg! just a few nits. Comment at: clang-tools-extra/clangd/index/dex/Iterator.cpp:109 +private: + /// Restores class invariants: each child should point to the same element.

[PATCH] D50045: [clangd] Report diagnostics even if WantDiags::No AST was reused

ioeric accepted this revision. ioeric added inline comments. This revision is now accepted and ready to land. Comment at: clangd/TUScheduler.cpp:381 + DiagsWereReported = PrevDiagsWereReported; + if (DiagsWereReported) { +// Take a shortcut and don't report the

[PATCH] D50045: [clangd] Report diagnostics even if WantDiags::No AST was reused

ioeric added a comment. Overall LG, just a suggestion to make the code a bit easier to follow. Comment at: clangd/TUScheduler.cpp:379 + +if (DiagsWereReported) { // Take a shortcut and don't build the AST if neither the inputs nor the The implicit

[PATCH] D48341: [clang-doc] Refactoring mapper to map by scope

ioeric added inline comments. Comment at: clang-tools-extra/clang-doc/Representation.cpp:49 +template +int isChildMergeNecessary(std::vector , T ) { + for (unsigned long I = 0; I < Children.size(); I++) { The function name doesn't describe what it does. Maybe

[PATCH] D50154: [clangd] capitalize diagnostic messages if '-capitialize-diagnostic-text' is provided

ioeric accepted this revision. ioeric added a comment. This revision is now accepted and ready to land. lgtm Comment at: clangd/Diagnostics.cpp:149 +/// Capitalizes the first word in the diagnostic's message. +std::string capitalizeDiagnosticText(std::string Message) { + if

[PATCH] D50154: [clangd] capitalize diagnostic messages if '-capitialize-diagnostic-text' is provided

ioeric added a comment. Is it possible for clangd to always capitalize diagnostics if `-capitialize-diagnostic-text` is found in the compile comand? Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D50154 ___ cfe-commits mailing list

[PATCH] D48341: [clang-doc] Refactoring mapper to map by scope

ioeric accepted this revision. ioeric added inline comments. This revision is now accepted and ready to land. Comment at: clang-tools-extra/test/clang-doc/bc-linkage.cpp:106 +// CHECK-0-NEXT: +// CHECK-0-NEXT: +// CHECK-0-NEXT:blob data = 'InnerClass'

[PATCH] D50154: [clangd] capitalize diagnostic messages if '-capitialize-diagnostic-text' is provided

ioeric added a comment. In https://reviews.llvm.org/D50154#1185545, @ilya-biryukov wrote: > Maybe we could simply always capitalize the messages? Any cons to > capitalizing error messages? > @simark, @malaperle, @ioeric, @hokein, WDYT? Oh sorry, I thought `-capitialize-diagnostic-text` was

[PATCH] D49991: [clangd] Do not build AST if no diagnostics were requested

ioeric accepted this revision. ioeric added a comment. This revision is now accepted and ready to land. lgtm Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D49991 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D50517: [clangd] Generate incomplete trigrams for the Dex index

ioeric accepted this revision. ioeric added a comment. This revision is now accepted and ready to land. lg! Thanks for the changes! Comment at: clang-tools-extra/clangd/index/dex/Trigram.cpp:147 + if (Chars.size() == 3) { +const auto Trigram = +

[PATCH] D50517: [clangd] Generate incomplete trigrams for the Dex index

ioeric added inline comments. Comment at: clang-tools-extra/clangd/index/dex/Trigram.cpp:33 + +void insertChars(DenseSet , std::string Chars) { + const auto Trigram = Token(Token::Kind::Trigram, Chars); This is probably neater as a lambda in

[PATCH] D50517: [clangd] Generate incomplete trigrams for the Dex index

ioeric added inline comments. Comment at: clang-tools-extra/clangd/index/dex/Trigram.cpp:31 +/// use it as the special symbol. +const auto END_MARKER = '$'; + nit: s/auto/char/ Maybe just use `static` instead of an anonymous namespace just for this.

[PATCH] D50517: [clangd] Generate incomplete trigrams for the Dex index

ioeric added inline comments. Comment at: clang-tools-extra/clangd/index/dex/Trigram.cpp:83 + + bool FoundFirstHead = false; + // Iterate through valid seqneces of three characters Fuzzy Matcher can ioeric wrote: > It's probably unclear what `FoundFirstHead`

[PATCH] D50446: [clangd] Record the file being processed in a TUScheduler thread in context.

This revision was automatically updated to reflect the committed changes. Closed by commit rL339320: [clangd] Record the file being processed in a TUScheduler thread in context. (authored by ioeric, committed by ). Herald added a subscriber: llvm-commits. Repository: rL LLVM

[PATCH] D50385: [clangd] Collect symbol occurrences from AST.

ioeric added a comment. In https://reviews.llvm.org/D50385#1193545, @hokein wrote: > In https://reviews.llvm.org/D50385#1191914, @ioeric wrote: > > > 2 high-level questions: > > > > 1. What's the reason for having a separate `SymbolOccurrenceSlab`? Could > > store occurrences as extra payload

[PATCH] D50500: [clangd] Allow consuming limited number of items

ioeric added a comment. Could you add test? ;) Comment at: clang-tools-extra/clangd/index/dex/Iterator.cpp:223 std::vector Result; - for (; !It.reachedEnd(); It.advance()) + for (size_t Retreived = 0; !It.reachedEnd() && Retreived < Limit; + It.advance())

[PATCH] D50337: [clangd] DexIndex implementation prototype

ioeric added inline comments. Comment at: clang-tools-extra/clangd/index/MemIndex.h:45 // Index is a set of symbols that are deduplicated by symbol IDs. - // FIXME: build smarter index structure. llvm::DenseMap Index; I think this FIXME still applies

[PATCH] D50385: [clangd] Collect symbol occurrences from AST.

ioeric added a comment. 2 high-level questions: 1. What's the reason for having a separate `SymbolOccurrenceSlab`? Could store occurrences as extra payload of `Symbol`? 2. Could we merge `SymbolOccurrenceCollector` into the existing `SymbolCollector`? They look a lot alike. Having another

[PATCH] D50446: [clangd] Record the currently active file in context for codeCompletion and findDefinitions.

ioeric added a comment. In https://reviews.llvm.org/D50446#1192282, @ilya-biryukov wrote: > Short summary of the offline discussion: I suggest adding this parameter into > the corresponding requests of the index (FuzzyFindRequest, > FindDefinitionsRequest) instead of stashing it in the

[PATCH] D50446: [clangd] Record the currently active file in context for codeCompletion and findDefinitions.

ioeric updated this revision to Diff 159746. ioeric marked 3 inline comments as done. ioeric added a comment. Herald added a subscriber: javed.absar. - Addressed review comments. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D50446 Files: clangd/TUScheduler.cpp

[PATCH] D50446: [clangd] Record the currently active file in context for codeCompletion and findDefinitions.

ioeric added a comment. Ok, I am convinced :) Putting the context key into TUScheduler.cpp and exposing a static method to access it sound like a better idea afterall. Thanks for the suggestions! Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D50446

[PATCH] D50727: [clangd] Fetch documentation from the Index during signature help

ioeric added inline comments. Comment at: clangd/CodeComplete.cpp:755 + }); + log("SigHelp: requested docs for {0} symbols from the index, got {1} " + "symbols with non-empty docs in the response", drive by: I think this should be `vlog` or

[PATCH] D50740: [SourceManager] isPointWithin: avoid using isBeforeInTranslationUnit, compare buffer offsets directly for lexical correctness

ioeric added inline comments. Comment at: lib/Basic/SourceManager.cpp:2035 + "Passed invalid source location!"); + assert(Start.isFileID() && End.isFileID() && Loc.isFileID() && + "Passed non-file source location!"); Why do we disallow locations

[PATCH] D50727: [clangd] Fetch documentation from the Index during signature help

ioeric added inline comments. Comment at: clangd/CodeComplete.cpp:755 + }); + log("SigHelp: requested docs for {0} symbols from the index, got {1} " + "symbols with non-empty docs in the response", hokein wrote: > ioeric wrote: > >

[PATCH] D50337: [clangd] DexIndex implementation prototype

ioeric added a comment. In https://reviews.llvm.org/D50337#1198914, @kbobyrev wrote: > Don't separate the logic for "long" and "short" queries: > https://reviews.llvm.org/D50517 (https://reviews.llvm.org/rCTE339548) > introduced incomplete trigrams which can be used on for "short" queries,

[PATCH] D50726: [clangd] Show function documentation in sigHelp

ioeric added inline comments. Comment at: clangd/CodeCompletionStrings.cpp:81 +std::string +getDocComment(const ASTContext , + const CodeCompleteConsumer::OverloadCandidate , The function doesn't seem to carry its weight, and the difference from the

[PATCH] D50727: [clangd] Fetch documentation from the Index during signature help

ioeric added inline comments. Comment at: clangd/CodeComplete.cpp:755 + }); + log("SigHelp: requested docs for {0} symbols from the index, got {1} " + "symbols with non-empty docs in the response", ilya-biryukov wrote: > ioeric wrote: > >

[PATCH] D50835: [clangd] Add parantheses while auto-completing functions.

ioeric added inline comments. Comment at: clangd/CodeComplete.h:85 + + /// Enables cursor to be moved around according to completions needs even when + /// snippets are disabled. For example selecting a function with parameters as IIRC, the goal of this

[PATCH] D50700: [clangd] Generate better incomplete bigrams for the Dex index

ioeric added inline comments. Comment at: clang-tools-extra/unittests/clangd/DexIndexTests.cpp:324 + EXPECT_THAT(generateQueryTrigrams("u_p"), trigramsAre({"up$"})); + EXPECT_THAT(generateQueryTrigrams("_u_p"), trigramsAre({"_u_"})); kbobyrev wrote: > ioeric

[PATCH] D50337: [clangd] DexIndex implementation prototype

ioeric added inline comments. Comment at: clang-tools-extra/clangd/index/dex/DexIndex.cpp:25 + std::vector> ) { + // First, sort retrieved symbols by the cumulative score to apply callbacks + // in the order of descending score. Why is this

[PATCH] D50689: [clangd] NFC: Improve Dex Iterators debugging traits

ioeric accepted this revision. ioeric added inline comments. This revision is now accepted and ready to land. Comment at: clang-tools-extra/clangd/index/dex/Iterator.h:102 /// ChildN is N-th iterator child. Raw iterators over PostingList are - /// represented as "[ID1, ID2,

[PATCH] D50700: [clangd] Generate better incomplete bigrams for the Dex index

ioeric added inline comments. Comment at: clang-tools-extra/unittests/clangd/DexIndexTests.cpp:324 + EXPECT_THAT(generateQueryTrigrams("u_p"), trigramsAre({"up$"})); + EXPECT_THAT(generateQueryTrigrams("_u_p"), trigramsAre({"_u_"})); I'm not sure if this is

[PATCH] D50847: [clangd] Add callbacks on parsed AST in addition to parsed preambles

ioeric added a comment. > Dynamic index misses important information from the body of the file, e.g. > locations of definitions > XRefs cannot be collected at all, since we can only obtain full information > for the current file (preamble is parsed with skipped function bodies, > therefore

[PATCH] D50726: [clangd] Show function documentation in sigHelp

ioeric accepted this revision. ioeric added a comment. This revision is now accepted and ready to land. lgtm Comment at: clangd/CodeCompletionStrings.cpp:52 // get this declaration, so we don't show documentation in that case. if (Result.Kind !=

[PATCH] D50727: [clangd] Fetch documentation from the Index during signature help

ioeric added inline comments. Comment at: clangd/CodeComplete.cpp:755 + }); + log("SigHelp: requested docs for {0} symbols from the index, got {1} " + "symbols with non-empty docs in the response", ilya-biryukov wrote: > ioeric wrote: > >

[PATCH] D50726: [clangd] Show function documentation in sigHelp

ioeric added inline comments. Comment at: clangd/CodeCompletionStrings.cpp:52 // get this declaration, so we don't show documentation in that case. if (Result.Kind != CodeCompletionResult::RK_Declaration) return ""; ilya-biryukov wrote: > ioeric

[PATCH] D50331: [clangd] *Prototype* Drop dynamic index symbols from files that are already indexed in static index.

ioeric created this revision. ioeric added reviewers: ilya-biryukov, hokein. Herald added subscribers: cfe-commits, arphaman, jkorous, MaskRay. Currently, dynamic index collects symbols for the entire TU for each open/active file. When static index is enabled, this can be wasteful as

[PATCH] D50331: [clangd] *Prototype* Drop dynamic index symbols from files that are already indexed in static index.

ioeric added a comment. Code is not polished. I'd like to get some high-level feedback before proceeding further and splitting this into smaller patches. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D50331 ___ cfe-commits mailing

[PATCH] D50375: [clangd] Share getSymbolID implementation.

ioeric accepted this revision. ioeric added inline comments. This revision is now accepted and ready to land. Comment at: clangd/AST.cpp:59 + llvm::SmallString<128> USR; + if (index::generateUSRForDecl(D, USR)) { +return None; nit: no braces

<    3   4   5   6   7   8   9   10   11   12   >