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

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

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

ioeric created this revision. ioeric added a reviewer: ilya-biryukov. Herald added subscribers: cfe-commits, arphaman, jkorous, MaskRay. This allows implementations like different symbol indexes to know what the current active file is. For example, some customized index implementation might decide

[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 here.

[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 context.

[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 clangd/TUSch

[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] 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 https://reviews.ll

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

[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] D50500: [clangd] Allow consuming limited number of items

ioeric accepted this revision. ioeric added inline comments. This revision is now accepted and ready to land. Comment at: clang-tools-extra/unittests/clangd/DexIndexTests.cpp:252 + auto DocIterator = create(L0); + EXPECT_THAT(consume(*DocIterator, 42), ElementsAre(4, 7, 8, 20,

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

ioeric added a comment. Thanks for the patch! In https://reviews.llvm.org/D50517#1194955, @kbobyrev wrote: > Complete the tests, finish the implementation. > > One thought about prefix match suggestion: we should either make it more > explicit for the index (e.g. introduce `prefixMatch` and dis

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

ioeric added a comment. In https://reviews.llvm.org/D50517#1194976, @kbobyrev wrote: > As discussed offline with @ilya-biryukov, the better approach would be to > prefix match first symbols of each distinct identifier piece instead of > prefix matching (just looking at the first letter of the i

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

ioeric added a comment. In https://reviews.llvm.org/D50517#1194976, @kbobyrev wrote: > As discussed offline with @ilya-biryukov, the better approach would be to > prefix match first symbols of each distinct identifier piece instead of > prefix matching (just looking at the first letter of the i

[PATCH] D50337: [clangd] DexIndex implementation prototype

ioeric added inline comments. Comment at: clang-tools-extra/clangd/index/dex/DexIndex.cpp:29 + // might be more efficient. + std::sort(begin(*Syms), end(*Syms), [](const Symbol *LHS, const Symbol *RHS) { +return quality(*LHS) > quality(*RHS); Calculating `

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

ioeric added inline comments. Comment at: clang-tools-extra/clangd/index/dex/Trigram.cpp:74 +// symbol of the identifier. +if (!FoundFirstSymbol) { + FoundFirstSymbol = true; kbobyrev wrote: > ioeric wrote: > > Could this be pulled out of the loop? I

[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 &UniqueTrigrams, std::string Chars) { + const auto Trigram = Token(Token::Kind::Trigram, Chars); This is probably neater as a lambda in `gene

[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` an

[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 = +Token(To

[PATCH] D50702: [clangd] NFC: Cleanup clangd help message

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

[PATCH] D50707: NFC: Enforce good formatting across multiple clang-tools-extra files

ioeric added a comment. Fixing header guards and file comments is fine, but I'm not a fan of reformatting-only changes in source code as they tends to make `git blame` harder. https://reviews.llvm.org/D50707 ___ cfe-commits mailing list cfe-commit

[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 `d

[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

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

ioeric updated this revision to Diff 161537. ioeric added a comment. - Make sure completion result callback can be called even if the unused speculative request has not finished. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D50962 Files: clangd/ClangdServer.cpp clangd/Cla

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

ioeric updated this revision to Diff 161651. ioeric marked an inline comment as done. ioeric added a comment. - Improve tests. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D50962 Files: clangd/ClangdServer.cpp clangd/ClangdServer.h clangd/CodeComplete.cpp clangd/CodeCo

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

ioeric added inline comments. Comment at: unittests/clangd/CodeCompleteTests.cpp:1710 + $bol^ + ab$ab^ + x.ab$dot^ kadircet wrote: > Maybe an EXPECT for this one as well? Nice catch! Repository: rCTE Clang Tools Extra https://reviews.llvm.org/

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

ioeric added inline comments. Comment at: lib/Basic/SourceManager.cpp:1705 // If we haven't found what we want yet, try again, but this time stat() // each of the files in case the files have changed since we originally Do we also need this in `findFileID

[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: ilya-biryukov wrote: > ioeric wrote: > > As chatted offline, I have questions about the motivation o

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

ioeric added inline comments. Comment at: clang-tools-extra/clangd/index/MemIndex.h:50 +// FIXME(kbobyrev): Document this one. +std::shared_ptr> Please add documentation. Comment at: clang-tools-extra/clangd/tool/ClangdMain.cpp:96 +namespac

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

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

[PATCH] D50898: [clangd] Suggest code-completions for overriding base class virtual methods.

ioeric added a comment. In https://reviews.llvm.org/D50898#1205756, @hokein wrote: > I think it is reasonable to make Sema support suggesting override methods, > instead of implementing it in clangd side? drive-by: +1 to this. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D5

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

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

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

ioeric added inline comments. Comment at: lib/Basic/SourceManager.cpp:1705 // If we haven't found what we want yet, try again, but this time stat() // each of the files in case the files have changed since we originally arphaman wrote: > ioeric wrote: > >

[PATCH] D51088: [clangd] Get rid of regexes in CanonicalIncludes

ioeric added inline comments. Comment at: clangd/index/CanonicalIncludes.h:44 /// Maps all files matching \p RE to \p CanonicalPath - void addRegexMapping(llvm::StringRef RE, llvm::StringRef CanonicalPath); + void addSuffixMapping(llvm::StringRef Suffix, llvm::StringRef Cano

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

ioeric updated this revision to Diff 161937. ioeric added a comment. - minor cleanup. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D50962 Files: clangd/ClangdServer.cpp clangd/ClangdServer.h clangd/CodeComplete.cpp clangd/CodeComplete.h clangd/index/Index.cpp clang

[PATCH] D51088: [clangd] Get rid of regexes in CanonicalIncludes

ioeric accepted this revision. ioeric added inline comments. This revision is now accepted and ready to land. Comment at: clangd/index/CanonicalIncludes.cpp:24 + int Components = 0; + for (auto It = llvm::sys::path::begin(Suffix), +End = llvm::sys::path::end(Suffix)

[PATCH] D51155: [clangd] Allow to merge symbols on-the-fly in global-symbol-builder

ioeric added a comment. Overall looks good to me. Comment at: clangd/global-symbol-builder/GlobalSymbolBuilderMain.cpp:61 +/// rely on MR use-case to work properly. +llvm::cl::init(false)); + AFAIK, this flag would basically depend on the executor being

[PATCH] D51155: [clangd] Allow to merge symbols on-the-fly in global-symbol-builder

ioeric added inline comments. Comment at: clangd/global-symbol-builder/GlobalSymbolBuilderMain.cpp:61 +/// rely on MR use-case to work properly. +llvm::cl::init(false)); + ilya-biryukov wrote: > ioeric wrote: > > AFAIK, this flag would basically depend on

[PATCH] D46000: [AST] Added a helper to extract a user-friendly text of a comment.

ioeric added a comment. Thanks for adding the tests! Comment at: include/clang/AST/RawCommentList.h:138 + /// the overload with ASTContext in the rest of the code. + std::string getFormattedText(const SourceManager &SourceMgr, + DiagnosticsEngine

[PATCH] D46176: Add SourceManagerForFile helper which sets up SourceManager and dependencies for a single file with code snippet

ioeric updated this revision to Diff 146004. ioeric marked 4 inline comments as done. ioeric added a comment. - address review comments. Repository: rC Clang https://reviews.llvm.org/D46176 Files: include/clang/Basic/SourceManager.h lib/Basic/SourceManager.cpp lib/Format/Format.cpp l

[PATCH] D46176: Add SourceManagerForFile helper which sets up SourceManager and dependencies for a single file with code snippet

This revision was automatically updated to reflect the committed changes. Closed by commit rL331923: Add SourceManagerForFile helper which sets up SourceManager and dependencies… (authored by ioeric, committed by ). Herald added a subscriber: llvm-commits. Repository: rL LLVM https://reviews.l

[PATCH] D46496: [Tooling] Pull #include manipulation code from clangFormat into libToolingCore.

ioeric updated this revision to Diff 146012. ioeric added a comment. - Merged with origin/master Repository: rC Clang https://reviews.llvm.org/D46496 Files: include/clang/Format/Format.h include/clang/Tooling/Core/HeaderIncludes.h lib/Format/Format.cpp lib/Tooling/Core/CMakeLists.txt

[PATCH] D46496: [Tooling] Pull #include manipulation code from clangFormat into libToolingCore.

ioeric updated this revision to Diff 146042. ioeric added a comment. - Merged with origin/master Repository: rC Clang https://reviews.llvm.org/D46496 Files: include/clang/Format/Format.h include/clang/Tooling/Core/HeaderIncludes.h lib/Format/Format.cpp lib/Tooling/Core/CMakeLists.txt

[PATCH] D46497: [clangd] Populate #include insertions as additional edits in completion items.

ioeric added a comment. In https://reviews.llvm.org/D46497#1089755, @sammccall wrote: > I'm concerned about the scope of this patch - it does too many things and > touches too many files for me to feel comfortable that I understand it well > enough to review. > Is it possible to split it up? (

[PATCH] D46497: [clangd] Populate #include insertions as additional edits in completion items.

ioeric updated this revision to Diff 146045. ioeric marked 15 inline comments as done. ioeric added a comment. - Merged with origin/master - Address review comments. - Revert unintended change. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D46497 Files: clangd/ClangdLSPServer

[PATCH] D46497: [clangd] Populate #include insertions as additional edits in completion items.

ioeric updated this revision to Diff 146046. ioeric added a comment. - Minor cleanup Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D46497 Files: clangd/ClangdLSPServer.cpp clangd/ClangdServer.cpp clangd/ClangdServer.h clangd/ClangdUnit.cpp clangd/ClangdUnit.h clangd

[PATCH] D46670: [clangd] Move helpers that convert Replacements to TextEdits to SourceCode.h

ioeric created this revision. ioeric added a reviewer: ilya-biryukov. Herald added subscribers: cfe-commits, jkorous, MaskRay, klimek. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D46670 Files: clangd/ClangdLSPServer.cpp clangd/SourceCode.cpp clangd/SourceCode.h Index: c

[PATCH] D46675: [clangd] Add helper for collecting #include directives in file.

ioeric created this revision. ioeric added a reviewer: sammccall. Herald added subscribers: cfe-commits, jkorous, MaskRay, ilya-biryukov, klimek. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D46675 Files: clangd/ClangdUnit.cpp clangd/ClangdUnit.h clangd/Headers.cpp clang

[PATCH] D46675: [clangd] Add helper for collecting #include directives in file.

ioeric updated this revision to Diff 146074. ioeric added a comment. - Rebase on origin/master Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D46675 Files: clangd/ClangdLSPServer.cpp clangd/ClangdUnit.cpp clangd/ClangdUnit.h clangd/Headers.cpp clangd/Headers.h clangd

[PATCH] D46675: [clangd] Add helper for collecting #include directives in file.

ioeric updated this revision to Diff 146075. ioeric added a comment. - Unmerge https://reviews.llvm.org/D46670 Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D46675 Files: clangd/ClangdUnit.cpp clangd/ClangdUnit.h clangd/Headers.cpp clangd/Headers.h clangd/XRefs.cpp I

[PATCH] D46676: [clangd] Remove LSP command-based #include insertion.

ioeric created this revision. ioeric added reviewers: ilya-biryukov, sammccall. Herald added subscribers: cfe-commits, jkorous, MaskRay, klimek. clangd will populate #include insertions as addtionalEdits in completion items. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D46676

[PATCH] D46670: [clangd] Move helpers that convert Replacements to TextEdits to SourceCode.h

ioeric updated this revision to Diff 146082. ioeric added a comment. - [clangd] Add helper for collecting #include directives in file. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D46670 Files: clangd/ClangdLSPServer.cpp clangd/SourceCode.cpp clangd/SourceCode.h Index:

[PATCH] D46497: [clangd] Populate #include insertions as additional edits in completion items.

ioeric updated this revision to Diff 146083. ioeric added a comment. - Rebase on https://reviews.llvm.org/D46670, https://reviews.llvm.org/D46675, https://reviews.llvm.org/D46676 Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D46497 Files: clangd/ClangdServer.cpp clangd/Cod

[PATCH] D46497: [clangd] Populate #include insertions as additional edits in completion items.

ioeric added a comment. This has been split and now depends on https://reviews.llvm.org/D46670, https://reviews.llvm.org/D46675, https://reviews.llvm.org/D46676. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D46497 ___ cfe-commits

[PATCH] D46676: [clangd] Remove LSP command-based #include insertion.

ioeric added a comment. In https://reviews.llvm.org/D46676#1095713, @ilya-biryukov wrote: > This LG, but we should first roll out the `additionalEdits` change. > Aren't the dependencies reversed in the dependency stack, i.e. this change > should be the last one? You are right ;) I got the dep

[PATCH] D46670: [clangd] Move helpers that convert Replacements to TextEdits to SourceCode.h

ioeric updated this revision to Diff 146303. ioeric marked 2 inline comments as done. ioeric added a comment. - Got rid of a helper for std::vector Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D46670 Files: clangd/ClangdLSPServer.cpp clangd/SourceCode.cpp clangd/SourceCo

[PATCH] D46670: [clangd] Move helpers that convert Replacements to TextEdits to SourceCode.h

ioeric added inline comments. Comment at: clangd/SourceCode.h:62 +std::vector +replacementsToEdits(StringRef Code, +const std::vector &Replacements); ilya-biryukov wrote: > Do we really need to expose separate functions for > `vector` and `to

[PATCH] D46670: [clangd] Move helpers that convert Replacements to TextEdits to SourceCode.h

This revision was automatically updated to reflect the committed changes. Closed by commit rL332089: [clangd] Move helpers that convert Replacements to TextEdits to SourceCode.h (authored by ioeric, committed by ). Herald added a subscriber: llvm-commits. Repository: rL LLVM https://reviews.ll

[PATCH] D46751: [clangd] Filter out private proto symbols in SymbolCollector.

ioeric created this revision. ioeric added a reviewer: ilya-biryukov. Herald added subscribers: cfe-commits, jkorous, MaskRay, klimek. This uses heuristics to identify private proto symbols. For example, top-level symbols whose name contains "_" are considered private. These symbols are not expect

[PATCH] D46676: [clangd] Remove LSP command-based #include insertion.

ioeric marked an inline comment as done. ioeric added inline comments. Comment at: unittests/clangd/ClangdTests.cpp:941 -TEST_F(ClangdVFSTest, InsertIncludes) { - MockFSProvider FS; ilya-biryukov wrote: > Do we test the same thing somewhere else (e.g. code co

[PATCH] D46758: [clang-format] Move #include related style to libToolingCore

ioeric created this revision. ioeric added a reviewer: ilya-biryukov. Herald added subscribers: cfe-commits, klimek. This will be shared by include insertion/deletion library. Repository: rC Clang https://reviews.llvm.org/D46758 Files: include/clang/Format/Format.h include/clang/Tooling/

[PATCH] D46751: [clangd] Filter out private proto symbols in SymbolCollector.

ioeric updated this revision to Diff 146338. ioeric marked 3 inline comments as done. ioeric added a comment. - Addressed a few comments. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D46751 Files: clangd/index/SymbolCollector.cpp unittests/clangd/SymbolCollectorTests.cpp

[PATCH] D46751: [clangd] Filter out private proto symbols in SymbolCollector.

ioeric added a comment. Thanks for sharing the example Marc! It's a bit surprising to see files that are not protobuf-generated named proto.h. I'm not a big fan of parsing file comment in proto. It seems a bit cumbersome and we might not be able (or too expensive) to do so for completion result

  1   2   3   4   5   6   7   8   9   10   >