[PATCH] D39050: Add index-while-building support to Clang

ioeric added a comment. First round of comments. Mostly around indexing actions and file records; I haven't started reviewing the data writing and storing code. I think it might make sense to split the index writing and storing logics into a separate patch, which should be possible if `writeUni

[PATCH] D40564: [clangd] Simplify common JSON-parsing patterns in Protocol.

ioeric accepted this revision. ioeric added a comment. This revision is now accepted and ready to land. lgtm Comment at: clangd/Protocol.cpp:56 +assert(*this && "Must check this is an object before calling parse()"); +if (const json::Expr *E = O->get(Prop)) { + ret

[PATCH] D40563: [SemaCodeComplete] Allow passing out scope specifiers in qualified-id completions via completion context.

ioeric updated this revision to Diff 124739. ioeric marked 4 inline comments as done. ioeric added a comment. - Address review comments. https://reviews.llvm.org/D40563 Files: include/clang/Sema/CodeCompleteConsumer.h lib/Sema/SemaCodeComplete.cpp Index: lib/Sema/SemaCodeComplete.cpp =

[PATCH] D40563: [SemaCodeComplete] Allow passing out scope specifiers in qualified-id completions via completion context.

ioeric added inline comments. Comment at: lib/Sema/SemaCodeComplete.cpp:4609 + if (SS.isInvalid()) { +CodeCompletionContext CC(CodeCompletionContext::CCC_Name); ilya-biryukov wrote: > ilya-biryukov wrote: > > Why do we alter this code path? > Maybe we shou

[PATCH] D40562: [Sema] Ignore decls in namespaces when global decls are not wanted.

ioeric updated this revision to Diff 124740. ioeric added a comment. - Clarify comment for includeGlobals() https://reviews.llvm.org/D40562 Files: include/clang/Sema/CodeCompleteConsumer.h lib/Sema/SemaCodeComplete.cpp lib/Sema/SemaLookup.cpp test/CodeCompletion/ignore-global-decls.cpp

[PATCH] D40548: [clangd] Prototyping index support and naive index-based global code completion. WIP

ioeric updated this revision to Diff 124745. ioeric added a comment. Herald added a subscriber: klimek. Merged with origin/master Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D40548 Files: clangd/ASTIndex.cpp clangd/ASTIndex.h clangd/CMakeLists.txt clangd/ClangdIndex.c

[PATCH] D40563: [SemaCodeComplete] Allow passing out scope specifiers in qualified-id completions via completion context.

ioeric added a comment. In https://reviews.llvm.org/D40563#939555, @arphaman wrote: > Could you please add a test? Any tip on how this should be tested? I couldn't find any existing unit test for either SemaCodeComplete or code completion context (under `clang/unittests`). It might be possibl

[PATCH] D40563: [SemaCodeComplete] Allow passing out scope specifiers in qualified-id completions via completion context.

ioeric added a comment. In https://reviews.llvm.org/D40563#939964, @arphaman wrote: > If nothing uses `getCXXScopeSpecifier` right now we can't really test it with > a clang or c-index-test regression test. A completion unit test could work > here. I don't think we actually have existing comple

[PATCH] D40654: [clangd] Set completion options per-request.

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/D40654 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.l

[PATCH] D40562: [Sema] Ignore decls in namespaces when global decls are not wanted.

ioeric added a comment. In https://reviews.llvm.org/D40562#941570, @arphaman wrote: > In https://reviews.llvm.org/D40562#940201, @ilya-biryukov wrote: > > > In https://reviews.llvm.org/D40562#939950, @arphaman wrote: > > > > > This change breaks cached completions for declarations in namespaces i

[PATCH] D40780: [clangd] Incorporate fuzzy-match into result rankings.

ioeric added inline comments. Comment at: clangd/ClangdUnit.cpp:377 + : Result(&Result), SymbolScore(score(Result)), FilterScore(FilterScore), +Score(FilterScore * SymbolScore) {} It might worth mentioning how well `FilterScore * SymbolScore` perfo

[PATCH] D40884: [Index] Add setPreprocessor member to IndexDataConsumer.

ioeric created this revision. This enables us to use information in Preprocessor when handling symbol occurrences. Repository: rC Clang https://reviews.llvm.org/D40884 Files: include/clang/Index/IndexDataConsumer.h include/clang/Index/IndexingAction.h lib/Index/IndexingAction.cpp too

[PATCH] D40485: [clangd] Introduced a Context that stores implicit data

ioeric added inline comments. Comment at: clangd/Context.h:63 +/// used as parents for some other Contexts. +class Context { +public: sammccall wrote: > I think we should strongly consider calling the class Ctx over Context. It's > going to appear in many functi

[PATCH] D40884: [Index] Add setPreprocessor member to IndexDataConsumer.

ioeric added a comment. In https://reviews.llvm.org/D40884#946506, @malaperle wrote: > You can get the preprocessor from the ASTContext, no? I don't think `ASTContext` contains preprocessor information. Repository: rC Clang https://reviews.llvm.org/D40884 ___

[PATCH] D40562: [Sema] Ignore decls in namespaces when global decls are not wanted.

ioeric updated this revision to Diff 125777. ioeric added a comment. - Add a new code-completion option IncludeNamespaceLevelDecls. For now, I only restrict this option work for qualified id completion to reduce the impact. Repository: rC Clang https://reviews.llvm.org/D40562 Files: inclu

[PATCH] D40562: [Sema] Ignore decls in namespaces when global decls are not wanted.

ioeric added a comment. In https://reviews.llvm.org/D40562#942521, @arphaman wrote: > In https://reviews.llvm.org/D40562#941753, @ilya-biryukov wrote: > > > In https://reviews.llvm.org/D40562#941570, @arphaman wrote: > > > > > I'm not actually 100% sure, but I would imagine that this one of the

[PATCH] D40548: [clangd] Prototyping index support and naive index-based global code completion. WIP

ioeric added a comment. Hi Marc, the patch is not ready for review yet. I am still cleaning up the prototype and will let you know when it's ready for review. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D40548 ___ cfe-commits mai

[PATCH] D40897: [clangd] Introduce a "Symbol" class.

ioeric added inline comments. Comment at: clangd/Symbol.h:23 + // The path of the source file where a symbol occurs. + std::string FilePath; + // The offset to the first character of the symbol from the beginning of the Is this relative or absolute?

[PATCH] D40884: [Index] Add setPreprocessor member to IndexDataConsumer.

ioeric updated this revision to Diff 125913. ioeric marked an inline comment as done. ioeric added a comment. - Removed a redundant #include Repository: rC Clang https://reviews.llvm.org/D40884 Files: include/clang/Index/IndexDataConsumer.h lib/Index/IndexingAction.cpp tools/libclang/C

[PATCH] D40884: [Index] Add setPreprocessor member to IndexDataConsumer.

This revision was automatically updated to reflect the committed changes. Closed by commit rL320030: [Index] Add setPreprocessor member to IndexDataConsumer. (authored by ioeric). Repository: rL LLVM https://reviews.llvm.org/D40884 Files: cfe/trunk/include/clang/Index/IndexDataConsumer.h

[PATCH] D40884: [Index] Add setPreprocessor member to IndexDataConsumer.

This revision was automatically updated to reflect the committed changes. Closed by commit rC320030: [Index] Add setPreprocessor member to IndexDataConsumer. (authored by ioeric). Changed prior to commit: https://reviews.llvm.org/D40884?vs=125913&id=125920#toc Repository: rC Clang https://r

[PATCH] D40548: [clangd] Prototyping index support and naive index-based global code completion. WIP

ioeric updated this revision to Diff 125928. ioeric added a comment. - More cleanups and merged with https://reviews.llvm.org/D40897 Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D40548 Files: clangd/CMakeLists.txt clangd/ClangdIndex.cpp clangd/ClangdIndex.h clangd/Clan

[PATCH] D40548: [clangd] Symbol index interfaces and index-based code completion.

ioeric updated this revision to Diff 125960. ioeric added a comment. - Use IncludeNamespaceLevelDecls option; fix some broken tests. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D40548 Files: clangd/CMakeLists.txt clangd/ClangdIndex.cpp clangd/ClangdIndex.h clangd/Clan

[PATCH] D40548: [clangd] Symbol index interfaces and index-based code completion.

ioeric updated this revision to Diff 125962. ioeric added a comment. Diff on https://reviews.llvm.org/D40897 instead origin/master! Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D40548 Files: clangd/CMakeLists.txt clangd/ClangdIndex.cpp clangd/ClangdIndex.h clangd/Clang

[PATCH] D40562: [Sema] Ignore decls in namespaces when global decls are not wanted.

ioeric added a comment. Ping. (fyi, you could find use of the new option in https://reviews.llvm.org/D40548) Repository: rC Clang https://reviews.llvm.org/D40562 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-b

[PATCH] D40563: [SemaCodeComplete] Allow passing out scope specifiers in qualified-id completions via completion context.

ioeric added a comment. Fyi, you could find use of the new API in https://reviews.llvm.org/D40548 I'd like to land this patch if there is no objection. https://reviews.llvm.org/D40563 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://l

[PATCH] D41001: [change-namespace] Fix crash when injected base-class name is used in friend declarations.

ioeric created this revision. Herald added subscribers: cfe-commits, klimek. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D41001 Files: change-namespace/ChangeNamespace.cpp unittests/change-namespace/ChangeNamespaceTests.cpp Index: unittests/change-namespace/ChangeNamespac

[PATCH] D41001: [change-namespace] Fix crash when injected base-class name is used in friend declarations.

This revision was automatically updated to reflect the committed changes. Closed by commit rL320139: [change-namespace] Fix crash when injected base-class name is used in friend… (authored by ioeric). Repository: rL LLVM https://reviews.llvm.org/D41001 Files: clang-tools-extra/trunk/change-

[PATCH] D40548: [clangd] Symbol index interfaces and index-based code completion.

ioeric added a comment. In https://reviews.llvm.org/D40548#949182, @malaperle wrote: > As a follow-up, here's the interface for querying the index that I am using > right now. It's meant to be able to retrieve from any kind of "backend", i.e. > in-memory, ClangdIndexDataStore, libIndexStore, et

[PATCH] D40952: [clangd] Convert lit code completion tests to unit-tests. NFC

ioeric accepted this revision. ioeric added a comment. lg Comment at: unittests/clangd/Matchers.h:54 + bool MatchAndExplain(const std::vector &V, + ::testing::MatchResultListener *L) const { +std::vector Matches(Matchers.size()); `ove

[PATCH] D40548: [clangd] Symbol index interfaces and index-based code completion.

ioeric updated this revision to Diff 126379. ioeric marked 8 inline comments as done. ioeric added a comment. - Address review comments. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D40548 Files: clangd/CMakeLists.txt clangd/ClangdIndex.cpp clangd/ClangdIndex.h clangd/

[PATCH] D40548: [clangd] Symbol index interfaces and index-based code completion.

ioeric added inline comments. Comment at: clangd/ClangdIndex.h:1 +//===--- ClangdIndex.h - Symbol indexes for clangd.---*- C++-*-===// +// sammccall wrote: > nit: `Clangd` prefix doesn't do much. > I'd suggest `Index.h` for the interface (including S

[PATCH] D39050: Add index-while-building support to Clang

ioeric added a comment. Thanks a lot for the changes! Some more comments inlined. Please mark addressed comments as done so that reviewers could know what to look :) Thanks! Comment at: include/clang/Frontend/CompilerInstance.h:187 + typedef std::function( + const Front

[PATCH] D40562: [Sema] Ignore decls in namespaces when global decls are not wanted.

ioeric added a comment. Ping? Repository: rC Clang https://reviews.llvm.org/D40562 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D40563: [SemaCodeComplete] Allow passing out scope specifiers in qualified-id completions via completion context.

This revision was automatically updated to reflect the committed changes. Closed by commit rL320471: [SemaCodeComplete] Allow passing out scope specifiers in qualified-id… (authored by ioeric, committed by ). Repository: rL LLVM https://reviews.llvm.org/D40563 Files: cfe/trunk/include/clang

[PATCH] D40897: [clangd] Introduce a "Symbol" class.

ioeric added inline comments. Comment at: clangd/index/Index.h:52 +private: + friend class llvm::DenseMapInfo; + warning: class 'DenseMapInfo' was previously declared as a struct [-Wmismatched-tags] Repository: rCTE Clang Tools Extra https://reviews.llvm.o

[PATCH] D40548: [clangd] Symbol index interfaces and index-based code completion.

ioeric updated this revision to Diff 126686. ioeric added a comment. Merged with origin/master and moved index-related files to index/ subdirectory. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D40548 Files: clangd/CMakeLists.txt clangd/ClangdLSPServer.cpp clangd/ClangdL

[PATCH] D54800: [clangd] Cleanup: stop passing around list of supported URI schemes.

ioeric created this revision. ioeric added a reviewer: sammccall. Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, MaskRay, ilya-biryukov. Instead of passing around a list of supported URI schemes in clangd, we expose an interface to convert a path to URI using any compatible s

[PATCH] D54800: [clangd] Cleanup: stop passing around list of supported URI schemes.

ioeric updated this revision to Diff 174929. ioeric added a comment. - remove unused include Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D54800 Files: clangd/ClangdLSPServer.cpp clangd/ClangdServer.cpp clangd/ClangdServer.h clangd/URI.cpp clangd/URI.h clangd/index

[PATCH] D54781: [clangd] Add 'Switch header/source' command in clangd-vscode

ioeric accepted this revision. ioeric added a comment. This revision is now accepted and ready to land. lgtm Could you run clang-format on the changed lines? Comment at: clangd/clients/clangd-vscode/package.json:81 +"command": "clangd-vscode.switchheadersource"

[PATCH] D54833: [clangd] Cleanup: use index file instead of header in workspace symbols lit test.

ioeric created this revision. ioeric added a reviewer: sammccall. Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, MaskRay, ilya-biryukov. The full path of the input header depends on the execution environment and may result in different behavior (e.g. when different URI scheme

[PATCH] D54800: [clangd] Cleanup: stop passing around list of supported URI schemes.

ioeric updated this revision to Diff 175048. ioeric marked 3 inline comments as done. ioeric added a comment. - address review comments Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D54800 Files: clangd/ClangdLSPServer.cpp clangd/ClangdServer.cpp clangd/ClangdServer.h c

[PATCH] D54833: [clangd] Cleanup: use index file instead of header in workspace symbols lit test.

This revision was automatically updated to reflect the committed changes. Closed by commit rCTE347466: [clangd] Cleanup: use index file instead of header in workspace symbols lit… (authored by ioeric, committed by ). Changed prior to commit: https://reviews.llvm.org/D54833?vs=175047&id=175051#t

[PATCH] D54800: [clangd] Cleanup: stop passing around list of supported URI schemes.

This revision was automatically updated to reflect the committed changes. Closed by commit rCTE347467: [clangd] Cleanup: stop passing around list of supported URI schemes. (authored by ioeric, committed by ). Changed prior to commit: https://reviews.llvm.org/D54800?vs=175048&id=175052#toc Repo

[PATCH] D54845: [clangd] Canonicalize file path in URIForFile.

ioeric created this revision. ioeric added a reviewer: sammccall. Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, MaskRay, ilya-biryukov. File paths in URIForFile can come from index or local AST. Path from index goes through URI transformation and the final path is resolved b

[PATCH] D54851: [clangd] Tune down scope boost for global scope

ioeric created this revision. ioeric added a reviewer: sammccall. Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, MaskRay, ilya-biryukov. This improves cross-namespace completions and has ignorable impact on other completion types. Metrics === OVERALL (excl. CROSS_NAME

[PATCH] D54851: [clangd] Tune down scope boost for global scope

This revision was automatically updated to reflect the committed changes. Closed by commit rCTE347548: [clangd] Tune down scope boost for global scope (authored by ioeric, committed by ). Changed prior to commit: https://reviews.llvm.org/D54851?vs=175106&id=175226#toc Repository: rCTE Clang

[PATCH] D54300: [clangd] Respect shouldIndexFile when collecting symbols.

ioeric added inline comments. Comment at: clangd/index/SymbolCollector.cpp:386 const Symbol *BasicSymbol = Symbols.find(*ID); - if (!BasicSymbol) // Regardless of role, ND is the canonical declaration. -BasicSymbol = addDeclaration(*ND, std::move(*ID)); - else if (isPref

[PATCH] D54894: [clangd] Enable auto-index behind a flag.

ioeric added a comment. lgtm (will let @kadircet stamp ) Comment at: test/clangd/background-index.test:19 +# Test the index is read from disk: delete code and restart clangd. +# This test currently fails as we don't read the index yet. +# RUN: rm %t/foo.cpp May

[PATCH] D54845: [clangd] Canonicalize file path in URIForFile.

ioeric updated this revision to Diff 175466. ioeric marked 7 inline comments as done. ioeric added a comment. address comments and rebase Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54845/new/ https://reviews.llvm.org/D54845 Files: clangd/Clang

[PATCH] D54845: [clangd] Canonicalize file path in URIForFile.

ioeric added a comment. Thanks for the review! PTAL Comment at: clangd/Protocol.h:74 + /// \p AbsPath is resolved to a canonical path corresponding to its URI. + URIForFile(llvm::StringRef AbsPath, llvm::StringRef HintPath = ""); + sammccall wrote: > sammccal

[PATCH] D54845: [clangd] Canonicalize file path in URIForFile.

ioeric added a comment. In D54845#1309589 , @sammccall wrote: > Mostly just places that should be updated HintPath -> TUPath. Changing the > whole codebase doesn't seem important, but in places that are touched... As chatted offline, `URIForFile` is clo

[PATCH] D54845: [clangd] Canonicalize file path in URIForFile.

This revision was automatically updated to reflect the committed changes. Closed by commit rCTE347739: [clangd] Canonicalize file path in URIForFile. (authored by ioeric, committed by ). Changed prior to commit: https://reviews.llvm.org/D54845?vs=175466&id=175642#toc Repository: rCTE Clang T

[PATCH] D55052: Fix junk output in clangd vscode plugin

ioeric added a comment. Thanks for the patch James! Adding folks who are currently using vscode+clangd to get more opinions. Comment at: clangd/clients/clangd-vscode/src/extension.ts:59 +// Avoid lots of junk in output +revealOutputChannelOn: vscodelc.RevealOut

[PATCH] D55052: Fix junk output in clangd vscode plugin

ioeric accepted this revision. ioeric added a comment. This revision is now accepted and ready to land. In D55052#1312766 , @hokein wrote: > In D55052#1312760 , @ilya-biryukov > wrote: > > > +1 to the change, this

[PATCH] D55061: [clangd] Penalize destructor and overloaded operators in code completion.

ioeric created this revision. ioeric added a reviewer: hokein. Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, MaskRay, ilya-biryukov. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D55061 Files: clangd/Quality.cpp clangd/Quality.h unittests/clangd/Quali

[PATCH] D55065: [clangd] Drop injected class name when class scope is not explicitly specified.

ioeric created this revision. ioeric added a reviewer: kadircet. Herald added subscribers: cfe-commits, arphaman, jkorous, MaskRay, ilya-biryukov. E.g. allow injected "A::A" in `using A::A^` but not in "A^". Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D55065 Files: clangd/

[PATCH] D55052: Fix junk output in clangd vscode plugin

ioeric added a comment. James, do you have commit access to llvm? If not, I'm happy to land the patch for you. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55052/new/ https://reviews.llvm.org/D55052 ___ cfe-commits mailing list cfe-commit

[PATCH] D55065: [clangd] Drop injected class name when class scope is not explicitly specified.

ioeric marked an inline comment as done. ioeric added inline comments. Comment at: unittests/clangd/CodeCompleteTests.cpp:419 +TEST(CompletionTest, SkipInjectedWhenUnqualified) { + EXPECT_THAT(completions("struct X { void f() { X^ }};").Completions, kadircet w

[PATCH] D55065: [clangd] Drop injected class name when class scope is not explicitly specified.

This revision was automatically updated to reflect the committed changes. Closed by commit rL347982: [clangd] Drop injected class name when class scope is not explicitly specified. (authored by ioeric, committed by ). Herald added a subscriber: llvm-commits. Repository: rL LLVM CHANGES SINCE L

[PATCH] D55061: [clangd] Penalize destructor and overloaded operators in code completion.

ioeric updated this revision to Diff 176075. ioeric marked an inline comment as done. ioeric added a comment. - Rename test name. Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55061/new/ https://reviews.llvm.org/D55061 Files: clangd/Quality.cpp

[PATCH] D55061: [clangd] Penalize destructor and overloaded operators in code completion.

ioeric added inline comments. Comment at: clangd/Quality.h:74 Keyword, +Operator, } Category = Unknown; hokein wrote: > Maybe name it `OverloadedOperator` to avoid confusion with other > non-overloaded operators like `?:` Ternary operator is not a sy

[PATCH] D55061: [clangd] Penalize destructor and overloaded operators in code completion.

This revision was automatically updated to reflect the committed changes. Closed by commit rCTE347983: [clangd] Penalize destructor and overloaded operators in code completion. (authored by ioeric, committed by ). Changed prior to commit: https://reviews.llvm.org/D55061?vs=176075&id=176076#toc

[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", ilya-biryukov wrote: > ioeric wrote: > > drive

[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: > > ilya-biryuko

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

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

[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: > > hokei

[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

[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] 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, too.

[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 c

[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> &Scores) { + // First, sort retrieved symbols by the cumulative score to apply callbacks + // in the order of descending score. Why is th

[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 not

[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 != CodeCompletionResult::RK_D

[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 wrote:

[PATCH] D50337: [clangd] DexIndex implementation prototype

ioeric added inline comments. Comment at: clang-tools-extra/clangd/index/dex/DexIndex.cpp:86 + const auto TrigramTokens = generateIdentifierTrigrams(Req.Query); + TopLevelChildren.push_back(createAnd(createTrigramIterators(TrigramTokens))); + `createTrigramIter

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

ioeric added inline comments. Comment at: clangd/CodeComplete.cpp:1377 + // Check whether function has any parameters or not. + LSP.textEdit->newText += SnippetSuffix.size() > 2 ? "(${0})" : "()"; +else There seems to be no guarantee on whether the

[PATCH] D50337: [clangd] DexIndex implementation prototype

ioeric added a comment. Almost LG! Just a few more nits. Comment at: clang-tools-extra/clangd/index/dex/DexIndex.cpp:87 + std::vector SymbolDocIDs; + std::priority_queue> Top; + nit: move `SymbolDocIDs` and `Top` closer to where they're used. ==

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

ioeric added inline comments. Comment at: clangd/CodeComplete.cpp:1377 + // Check whether function has any parameters or not. + LSP.textEdit->newText += SnippetSuffix.size() > 2 ? "(${0})" : "()"; +else kadircet wrote: > ioeric wrote: > > There seem

[PATCH] D50337: [clangd] DexIndex implementation prototype

ioeric added inline comments. Comment at: clang-tools-extra/clangd/index/dex/DexIndex.cpp:97 +} +// FIXME(kbobyrev): Add True iterator as soon as it's implemented otherwise. +// If TopLevelChildren vector will be empty it will trigger an assertion. A

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

ioeric added inline comments. Comment at: clangd/CodeComplete.cpp:1379 + Kind == CompletionItemKind::Method) && + SnippetSuffix.front() == '(' && SnippetSuffix.back() == ')') { + // Check whether function has any parameters or not. n

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

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/D50835 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.l

[PATCH] D50896: [clangd] Add xrefs LSP boilerplate implementation.

ioeric added a comment. Agreed with Ilya. I'd probably also make this depend on the ongoing implementation, as exposing LSP endpoints without proper implementation might be confusing to clangd users who only look at the the LSP endpoints. Users need to dig two levels of abstraction to find out

[PATCH] D50955: [clangd] Implement TRUE Iterator

ioeric added inline comments. Comment at: clang-tools-extra/clangd/index/dex/Iterator.cpp:230 +/// order to prevent additional memory consumption, it only stores the size of +/// this virtual posting list because posting lists of such density are likely +/// to consume a lot of m

[PATCH] D50956: [clangd] NFC: Cleanup Dex Iterator comments and simplify tests

ioeric accepted this revision. ioeric added a comment. This revision is now accepted and ready to land. lgtm https://reviews.llvm.org/D50956 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-c

[PATCH] D50955: [clangd] Implement TRUE Iterator

ioeric accepted this revision. ioeric added a comment. This revision is now accepted and ready to land. lg https://reviews.llvm.org/D50955 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-com

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

ioeric added inline comments. Herald added a subscriber: kadircet. Comment at: clangd/TUScheduler.h:66 + /// instead. + virtual void onMainAST(PathRef Path, ParsedAST &AST) = 0; +}; ilya-biryukov wrote: > hokein wrote: > > Does the callback get called every tim

[PATCH] D50960: [clangd] Add missing lock in the lookup.

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/D50960 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.l

[PATCH] D50961: [clangd] Simplify the code using UniqueStringSaver, NFC.

ioeric accepted this revision. ioeric added inline comments. This revision is now accepted and ready to land. Comment at: docs/clang-tidy/checks/abseil-duration-division.rst:1 +.. title:: clang-tidy - abseil-duration-division + Please revert. Repository: rCTE

[PATCH] D50962: [clangd] Speculative code completion index request before Sema is run.

ioeric created this revision. ioeric added reviewers: hokein, ilya-biryukov. Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, MaskRay. For index-based code completion, send an asynchronous speculative index request, based on the index request for the last code completion on the

[PATCH] D50889: [clangd] Make FileIndex aware of the main file

ioeric added inline comments. Comment at: clangd/ClangdServer.cpp:70 +// FIXME(ibiryukov): this should be a generic helper instead. +class NoopCallbacks : public ParsingCallbacks { public: Maybe provide noop implementations for `ParsingCallbacks::onPreambleAST()

[PATCH] D50926: [SourceManager] Extract 'findFileIDsForFile' from 'translateFile' to allow mapping from one file entry to multiple FileIDs

ioeric added inline comments. Comment at: include/clang/Basic/SourceManager.h:1533 + /// Looks through all the local and imported source locations to find the set + /// of FileIDs that correspond to the given entry. nit: put this closer to the closely related

[PATCH] D50502: [clangd] Initial cancellation mechanism for LSP requests.

ioeric added inline comments. Comment at: clangd/Cancellation.h:96 +/// checks using it to avoid extra lookups in the Context. +class CancellationToken { +public: As chatted offline, I have questions about the motivation of the `CancellationToken` class. Intuiti

[PATCH] D50337: [clangd] DexIndex implementation prototype

ioeric accepted this revision. ioeric added a comment. This revision is now accepted and ready to land. LG. Last few nits and then good to go. Comment at: clang-tools-extra/clangd/index/dex/DexIndex.cpp:97 +} +TopLevelChildren.push_back(createAnd(move(TrigramIterators))

[PATCH] D50966: Fix an undefined behavior when storing an empty StringRef.

ioeric added inline comments. Comment at: lib/Support/StringSaver.cpp:14 StringRef StringSaver::save(StringRef S) { + if (S.empty()) Is it possible to reuse `StringRef::copy(Allocator &)` here? Repository: rL LLVM https://reviews.llvm.org/D50966 _

[PATCH] D50897: [clangd] Allow using experimental Dex index

ioeric added inline comments. Herald added a subscriber: kadircet. Comment at: clang-tools-extra/clangd/index/Index.h:360 + template + static std::unique_ptr build(SymbolSlab Slab) { + struct Snapshot { I'd try to avoid this pattern as it implicitly requires

[PATCH] D50966: Fix an undefined behavior when storing an empty StringRef.

ioeric accepted this revision. ioeric added a comment. This revision is now accepted and ready to land. lg Comment at: lib/Support/StringSaver.cpp:14 StringRef StringSaver::save(StringRef S) { + if (S.empty()) hokein wrote: > ioeric wrote: > > Is it possibl

[PATCH] D50962: [clangd] Speculative code completion index request before Sema is run.

ioeric planned changes to this revision. ioeric added a comment. I just realized that `CodeCompleteFlow` would be blocked by the speculative index request even when index request is not needed (e.g. member access), because it owns the future object. This can make some completion requests slower

<    7   8   9   10   11   12   13   14   15   16   >