[PATCH] D52004: [clangd] Allow all LSP methods to signal cancellation via $/cancelRequest

sammccall updated this revision to Diff 165255. sammccall added a comment. Update ClangdLSPServer comment, document cancelRequest further. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D52004 Files: clangd/ClangdLSPServer.cpp clangd/ClangdLSPServer.h clangd/ClangdServer.c

[PATCH] D52004: [clangd] Allow all LSP methods to signal cancellation via $/cancelRequest

sammccall added a comment. In https://reviews.llvm.org/D52004#1232512, @kadircet wrote: > Wonder if we can still keep the onCancelRequest registry within > ProtocolHandler's scope, so that it is clear that we implement it. Other than > that seems fascinating, thanks! Hmm, I'm not sure how to

[PATCH] D51747: [clangd] Implement deprecation diagnostics with lower severity.

sammccall added inline comments. Comment at: clangd/ClangdServer.cpp:537 C->CommandLine.push_back("-resource-dir=" + ResourceDir); + C->CommandLine.push_back("-Wdeprecated-declarations"); return std::move(*C); kadircet wrote: > sammccall wrote: > > as note

[PATCH] D51982: [clangd] Introduce PostingList interface

sammccall added a comment. At a high level: making posting lists an abstract type adds another layer of indirection, which we can use to solve problems. It also has costs, mostly conceptual complexity. What problems are we solving? - if **we want to dynamically use a different representation fo

[PATCH] D52030: [clangd] Test benchmark if we can build it

sammccall created this revision. sammccall added a reviewer: kbobyrev. Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, MaskRay, ioeric, ilya-biryukov, mgorny. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D52030 Files: test/CMakeLists.txt test/clangd/inde

[PATCH] D52004: [clangd] Allow all LSP methods to signal cancellation via $/cancelRequest

This revision was automatically updated to reflect the committed changes. Closed by commit rL342135: [clangd] Allow all LSP methods to signal cancellation via $/cancelRequest (authored by sammccall, committed by ). Herald added a subscriber: llvm-commits. Repository: rL LLVM https://reviews.ll

[PATCH] D51747: [clangd] Show deprecation diagnostics

sammccall accepted this revision. sammccall added inline comments. This revision is now accepted and ready to land. Comment at: clangd/ClangdServer.cpp:534 - // Inject the resource dir. + // Inject the resource dir. These flags are working for both gcc and clang-cl + // driv

[PATCH] D52028: [clangd] Cleanup FuzzyFindRequest filtering limit semantics

sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land. Thanks! Comment at: clang-tools-extra/clangd/index/Index.h:477 /// - /// Returns true if there may be more results (limited by MaxCandidateCount). + /// Returns tr

[PATCH] D52030: [clangd] Test benchmark if we can build it

sammccall added a comment. In https://reviews.llvm.org/D52030#1233323, @kbobyrev wrote: > Looks good, thanks! > > While we're here: I'm wondering whether we should also introduce very basic > test which would just run `clangd-indexer` since it doesn't depend on > benchmarks and would be run on

[PATCH] D51747: [clangd] Show deprecation diagnostics

sammccall added a comment. Sorry for all the back and forth on this patch, but I have to ask... what's up with switching to lit tests? We've mostly started avoiding these as they're hard to maintain and debug (not to mention write... crazy sed tricks!) They're mostly used in places where we ne

[PATCH] D51747: [clangd] Show deprecation diagnostics

sammccall added a comment. In https://reviews.llvm.org/D51747#1233377, @kadircet wrote: > In https://reviews.llvm.org/D51747#1233371, @sammccall wrote: > > > Sorry for all the back and forth on this patch, but I have to ask... what's > > up with switching to lit tests? > > > > We've mostly start

[PATCH] D51982: [clangd] Introduce PostingList interface

sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land. Great! Comment at: clang-tools-extra/clangd/index/dex/Iterator.cpp:11 #include "Iterator.h" +#include "PostingList.h" #include And here ==

[PATCH] D51747: [clangd] Show deprecation diagnostics

sammccall accepted this revision. sammccall added a comment. Awesome, thank you. Just a couple of last nits. Comment at: unittests/clangd/ClangdTests.cpp:984 +std::vector Diagnostics) override { + std::lock_guard Lock(Mutex); + for(const Di

[PATCH] D51921: [VFS] vfs::directory_iterator yields path and file type instead of full Status

sammccall added a comment. (Just checking - IIUC neither of you are asking for changes, and this is waiting on review of the latest snapshot?) Repository: rC Clang https://reviews.llvm.org/D51921 ___ cfe-commits mailing list cfe-commits@lists.ll

[PATCH] D52071: [clangd] Don't override the preamble while completing inside it, it doesn't work.

sammccall created this revision. sammccall added a reviewer: ilya-biryukov. Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, MaskRay, ioeric. To stay fast, enable single-file-mode instead. This is fine since completions in the preamble are simple. The net effect for now is to

[PATCH] D52076: [CodeComplete] Add completions for filenames in #include directives.

sammccall created this revision. sammccall added a reviewer: ilya-biryukov. Herald added a subscriber: cfe-commits. The dir component ("somedir" in #include ) is considered fixed. We append "foo" to each directory on the include path, and then list its files. Completions are of the forms: ^-te

[PATCH] D51729: [Tooling] JSONCompilationDatabasePlugin infers compile commands for missing files

This revision was automatically updated to reflect the committed changes. Closed by commit rC342228: [Tooling] JSONCompilationDatabasePlugin infers compile commands for missing… (authored by sammccall, committed by ). Changed prior to commit: https://reviews.llvm.org/D51729?vs=164192&id=165472#

[PATCH] D52071: [clangd] Don't override the preamble while completing inside it, it doesn't work.

This revision was automatically updated to reflect the committed changes. Closed by commit rL342230: [clangd] Don't override the preamble while completing inside it, it doesn't… (authored by sammccall, committed by ). Herald added a subscriber: llvm-commits. Repository: rL LLVM https://reviews

[PATCH] D52071: [clangd] Don't override the preamble while completing inside it, it doesn't work.

This revision was automatically updated to reflect the committed changes. Closed by commit rCTE342230: [clangd] Don't override the preamble while completing inside it, it doesn't… (authored by sammccall, committed by ). Changed prior to commit: https://reviews.llvm.org/D52071?vs=165421&id=16547

[PATCH] D51921: [VFS] vfs::directory_iterator yields path and file type instead of full Status

This revision was automatically updated to reflect the committed changes. Closed by commit rC342232: [VFS] vfs::directory_iterator yields path and file type instead of full Status (authored by sammccall, committed by ). Changed prior to commit: https://reviews.llvm.org/D51921?vs=165250&id=16548

[PATCH] D52076: [CodeComplete] Add completions for filenames in #include directives.

sammccall added a comment. Thanks a lot, great comments! I haven't made changes yet (I will) but there's a few questions to work out first... Comment at: lib/Lex/Lexer.cpp:1931 + StringRef PartialPath(AfterQuote, CurPtr - 1 - AfterQuote); + auto Slash = Parti

[PATCH] D52076: [CodeComplete] Add completions for filenames in #include directives.

sammccall updated this revision to Diff 165593. sammccall marked 9 inline comments as done. sammccall added a comment. Unify common completion code from angled/quoted strings in Lexer. Handle #include paths with \ on windows (normalize them to /) Document why we picked particular extensions for he

[PATCH] D52076: [CodeComplete] Add completions for filenames in #include directives.

sammccall added a comment. Addressed the comments I was sure about. A couple of open questions in SemaCodeComplete about the formatting of the completions, but want to know what you think. Comment at: lib/Sema/SemaCodeComplete.cpp:8046 + !EC && It != vfs::directory_ite

[PATCH] D52078: [clangd] Store preamble macros in dynamic index.

sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land. This is definitely the right thing to do, thanks for finding it! I've got a long comment about how everything used to be simpler in the good old days :-) I'm itching to refactor a bit, bu

[PATCH] D49540: [clangd] FuzzyMatch exposes an API for its word segmentation. NFC

sammccall created this revision. sammccall added reviewers: ioeric, omtcyfz. Herald added subscribers: cfe-commits, arphaman, jkorous, MaskRay, ilya-biryukov. This is intended to be used for indexing, e.g. in https://reviews.llvm.org/D49417 Repository: rCTE Clang Tools Extra https://reviews

[PATCH] D49546: [clangd] Implement query iterators for Dex symbol index

sammccall added a comment. Thanks for sending this early! Rough interface comments - mostly looks good though! Comment at: clang-tools-extra/clangd/index/dex/QueryIterator.h:26 + +using PostingList = std::vector; + we should at least use a type alias for a DocI

[PATCH] D49540: [clangd] FuzzyMatch exposes an API for its word segmentation. NFC

This revision was automatically updated to reflect the committed changes. Closed by commit rL337527: [clangd] FuzzyMatch exposes an API for its word segmentation. NFC (authored by sammccall, committed by ). Herald added a subscriber: llvm-commits. Repository: rL LLVM https://reviews.llvm.org/D

[PATCH] D49591: [clangd] Introduce search Tokens and trigram generation algorithms

sammccall added inline comments. Comment at: clang-tools-extra/clangd/index/dex/Token.cpp:25 + Data.size() == 3 && "Trigram should contain three characters."); + switch (TokenKind) { + case Kind::Trigram: specializing the hash function looks like premat

[PATCH] D49591: [clangd] Introduce search Tokens and trigram generation algorithms

sammccall added inline comments. Comment at: clang-tools-extra/clangd/index/dex/Trigram.cpp:37 + + // Extract trigrams consisting of first characters of tokens sorted bytoken + // positions. Trigram generator is allowed to skip 1 word between each token. sammcc

[PATCH] D49543: [clangd] Penalize non-instance members when accessed via class instances.

sammccall accepted this revision. sammccall added inline comments. This revision is now accepted and ready to land. Comment at: clangd/CodeComplete.h:148 bool HasMore = false; + CodeCompletionContext::Kind SemaContext = CodeCompletionContext::CCC_Other; }; n

[PATCH] D48559: [clangd] refactoring for XPC transport layer [NFCI]

sammccall added a comment. Sorry for the delay here, and I'm sorry I haven't been into the patches in detail again yet - crazy week. After experiments, I am convinced the Transport abstraction patch can turn into something nice **if** we want XPC vs JSON to be a pure transport-level difference

[PATCH] D48071: [clangd] Add an option controlling caching of compile commands.

sammccall added a comment. In https://reviews.llvm.org/D48071#1171289, @ilya-biryukov wrote: > A recent change (https://reviews.llvm.org/D49267) is another indication that > caching might be doing more wrong than good. I assume the caching does not > give us much performance-wise, we only reque

[PATCH] D49546: [clangd] Implement query iterators for Dex symbol index

sammccall added a comment. A few more comments about the bits I understand, but waiting mostly on the documentation. Comment at: clang-tools-extra/clangd/index/dex/QueryIterator.cpp:155 + llvm::raw_ostream &dump(llvm::raw_ostream &OS) const override { +OS << "(&& "; +

[PATCH] D49658: [clangd] Index Interfaces for Xrefs

sammccall added a comment. Mostly LG. I think we can simplify in a couple of places, and you should decide if this patch is *implementing* the new index operation or not. (Currently it implements it for 1/3 implementations, which doesn't seem that useful). Comment at: clangd

[PATCH] D49657: [clangd] Make SymbolLocation => bool conversion explicitly.

sammccall added a subscriber: hokein. sammccall added a comment. Or make operator bool explicit Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D49657 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-b

[PATCH] D49591: [clangd] Introduce search Tokens and trigram generation algorithms

sammccall added a comment. Looks really nice! Only major issue is the query trigrams don't look right. Otherwise, some style nits and fixes that seem to have gotten lost. Comment at: clang-tools-extra/clangd/index/dex/Token.cpp:28 + +llvm::StringRef Token::data() const { return

[PATCH] D49667: [clangd] Tune down quality score for class constructors so that it's ranked after class types.

sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land. No opinion on the boost style thing. Comment at: unittests/clangd/QualityTests.cpp:198 EXPECT_LT(Macro.evaluate(), Default.evaluate()); + EXPECT_LT(Macro.evaluate()

[PATCH] D49719: [ClangFormat] Editor integrations inherit default style from clang-format binary

sammccall created this revision. sammccall added a reviewer: ioeric. Herald added a subscriber: cfe-commits. This allows downstream customizations to the default style to work without needing to also modify the editor integrations. Repository: rC Clang https://reviews.llvm.org/D49719 Files:

[PATCH] D49719: [ClangFormat] Editor integrations inherit default style from clang-format binary

sammccall marked an inline comment as done. sammccall added a comment. Thanks, good catch! Repository: rC Clang https://reviews.llvm.org/D49719 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo

[PATCH] D49719: [ClangFormat] Editor integrations inherit default style from clang-format binary

sammccall updated this revision to Diff 156974. sammccall added a comment. Remove one last -style flag. Repository: rC Clang https://reviews.llvm.org/D49719 Files: tools/clang-format/clang-format-sublime.py tools/clang-format/clang-format-test.el tools/clang-format/clang-format.el to

[PATCH] D49724: [VFS] Cleanups to VFS interfaces.

sammccall created this revision. sammccall added a reviewer: klimek. Herald added a subscriber: cfe-commits. - add comments clarifying semantics - Status::copyWithNewName(Status, Name) --> instance method - Status::copyWithNewName(fs::file_status, Name) --> constructor (it's not a copy) - File::g

[PATCH] D49546: [clangd] Implement query iterators for Dex symbol index

sammccall added a comment. This looks really nice. Iterator implementations can be simplified a bit I think. Comment at: clang-tools-extra/clangd/index/dex/Iterator.cpp:50 + OS << Documents[Index]; + if (Index + 1 != Documents.size()) +OS << ", "; ---

[PATCH] D49591: [clangd] Introduce search Tokens and trigram generation algorithms

sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land. Thanks for the polish! Last few nits, but feel free to land once you're happy. Comment at: clang-tools-extra/clangd/index/dex/Token.cpp:21 + +Token::Token(Kind TokenKin

[PATCH] D49267: [clangd] Watch for changes in compile_commands.json

sammccall added a comment. Just a couple of high-level comments here: - I'm not sure we can/should commit to supporting editor-based file watching forever. - One natural long-term direction would be to get this functionality into `JSONCompilationDatabase`, and clients of that don't have an LS

[PATCH] D49724: [VFS] Cleanups to VFS interfaces.

This revision was automatically updated to reflect the committed changes. Closed by commit rL337834: [VFS] Cleanups to VFS interfaces. (authored by sammccall, committed by ). Herald added a subscriber: llvm-commits. Repository: rL LLVM https://reviews.llvm.org/D49724 Files: cfe/trunk/includ

[PATCH] D49967: [clangd] Add command-line option to suppress the space and the circular dot prepended in a completion label.

sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land. Makes sense, thanks! Comment at: clangd/tool/ClangdMain.cpp:161 +static llvm::cl::opt NoHeaderInsertDecorators( +"no-header-insert-decorators", +llvm::cl::desc(

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

sammccall added a comment. Capitalizing sounds nice. +1 to just do it without an option. My favorite essay on this is http://neugierig.org/software/blog/2018/07/options.html Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D50154 ___

[PATCH] D42573: [clangd] The new threading implementation

sammccall added a comment. Very nice! Thanks for adding the docs to TUScheduler implementation, makes a big difference. Rest is almost all readability/comment bits. Substantive stuff is: - getUsedBytes() looks racy - I'm not sure we're choosing the right preamble My understanding is the thread

[PATCH] D41102: Setup clang-doc frontend framework

sammccall added a subscriber: bkramer. sammccall added a comment. In https://reviews.llvm.org/D41102#998180, @thakis wrote: > This should be in clang-tools-extra next to clang-tidy, clang-include-fixer, > clangd etc, not in the main compiler repo, right? I agree. I see there was earlier discus

[PATCH] D42915: [clangd] Use URIs in index symbols.

sammccall added a comment. This looks OK so far, where is it going? It doesn't make sense to put URIs in if we only ever use `file:`. I guess others will be produced by some kind of extension point on SymbolCollector. That will specify URI schemes we should try, allow you to replace the whole `

[PATCH] D42942: [clangd] Collect definitions when indexing.

sammccall created this revision. sammccall added reviewers: ioeric, hokein. Herald added subscribers: cfe-commits, jkorous-apple, ilya-biryukov, klimek. (This isn't done! Needs new tests, and some random cleanups should be split out. Looking for some early feedback.) Within a TU: - as now, colle

[PATCH] D42942: [clangd] Collect definitions when indexing.

sammccall added a comment. This does seem to get at least the simple cases right: ID: 0EF8AF4D08B11EBF3FFB8004CE702991B15F Name:SymbolsFromYAML Scope: 'clang::clangd::' SymInfo: Kind:Function Lang:C Canonica

[PATCH] D42915: [clangd] Use URIs in index symbols.

sammccall added a comment. Great, this all makes sense. I think we can/should make the scheme selection a bit more robust (we shouldn't crash if we get unexpected filenames). And... Uri or URI (I really think this is a usability issue - i had a scarring experience with a codebase that couldn't d

[PATCH] D42919: [clangd] Support simpler JSON-RPC stream parsing for lit tests.

sammccall marked an inline comment as done. sammccall added a comment. In https://reviews.llvm.org/D42919#998695, @ioeric wrote: > LGTM > > Have we kept a lit test that uses content-length? It's unclear from the patch. Yes, `protocol.test` tests the real protocol parser. (The other tests that u

[PATCH] D42919: [clangd] Support simpler JSON-RPC stream parsing for lit tests.

sammccall updated this revision to Diff 132960. sammccall marked an inline comment as done. sammccall added a comment. -test -> -lit-test Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D42919 Files: clangd/ClangdLSPServer.cpp clangd/ClangdLSPServer.h clangd/JSONRPCDispatch

[PATCH] D42573: [clangd] The new threading implementation

sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land. This is really great. Just one test nit. Comment at: unittests/clangd/ThreadingTests.cpp:34 + +scheduleIncrements(); +Tasks.waitForAll(); The c

[PATCH] D42942: [clangd] Collect definitions when indexing.

sammccall updated this revision to Diff 132985. sammccall marked 3 inline comments as done. sammccall added a comment. [clangd] Prefer data from TUs with symbol defn to data from TUs without. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D42942 Files: clangd/global-symbol-bui

[PATCH] D42942: [clangd] Collect definitions when indexing.

sammccall added a comment. Thanks for comments! (Still not done, adding tests) Comment at: clangd/global-symbol-builder/GlobalSymbolBuilderMain.cpp:67 + // XXX this is just to make running the tool fast during dev! + bool BeginInvocation(CompilerInstance &CI) override

[PATCH] D42964: [clangd] Add Revision field to Symbol.

sammccall added a comment. Discussed offline a bit: it's not clear that this field is going to be generally useful - we don't have a plan to read this from open-source code. (Google's internal index wants to be able to individually version symbols for distributed-system reasons, but we can add t

[PATCH] D42915: [clangd] Use URIs in index symbols.

sammccall accepted this revision. sammccall added inline comments. This revision is now accepted and ready to land. Comment at: unittests/clangd/SymbolCollectorTests.cpp:280 runSymbolCollector(Header.code(), /*Main=*/""); - EXPECT_THAT(Symbols, - UnorderedElement

[PATCH] D42942: [clangd] Collect definitions when indexing.

sammccall updated this revision to Diff 133024. sammccall added a comment. Add tests for SymbolCollector (finding def/decl locations) and merge. Found some bugs in SymbolCollector's locations - added fixmes. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D42942 Files: clangd/

[PATCH] D42942: [clangd] Collect definitions when indexing.

sammccall added a comment. OK, I think this is good to go now. Rebased against Eric's URI change and added tests. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D42942 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://l

[PATCH] D42942: [clangd] Collect definitions when indexing.

sammccall updated this revision to Diff 133501. sammccall marked 6 inline comments as done. sammccall added a comment. Address review comments. Make SymbolsToYAML take an ostream instead of returning a string. Define SymbolSlab::iterator so googletest will print it as a container. Repository:

[PATCH] D42942: [clangd] Collect definitions when indexing.

sammccall added a comment. Thanks for the comments! And sorry for the delay, I was haunted by useless gtest messages, which https://reviews.llvm.org/D43091 should fix. Comment at: clangd/global-symbol-builder/GlobalSymbolBuilderMain.cpp:159 + // Output phase: emit YAML for re

[PATCH] D42942: [clangd] Collect definitions when indexing.

sammccall updated this revision to Diff 133502. sammccall added a comment. Revert hack in global-symbol-builder to filter the input files. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D42942 Files: clangd/global-symbol-builder/GlobalSymbolBuilderMain.cpp clangd/index/Index

[PATCH] D43068: [clangd] Remove codeComplete that returns std::future<>

sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land. One down! I'd like to know what you think about a generic "block the call and capture the result" mechanism rather than method-specific wrappers. But if you're not convinced or just want

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

sammccall added a comment. Some comments on the insert side, which looks pretty good. I'll take another look at indexing tomorrow. Comment at: clangd/ClangdServer.cpp:465 +auto &HeaderSearchInfo = Clang->getPreprocessor().getHeaderSearchInfo(); +std::string Suggested =

[PATCH] D43065: [clangd] Remove threading-related code from ClangdUnit.h

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

[PATCH] D43122: [clangd] Fix crash when CompilerInvocation can't be created.

sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land. just nits Comment at: clangd/ClangdUnit.cpp:417 } assert(CI && "Couldn't create CompilerInvocation"); remove? Comment at: un

[PATCH] D43127: [clangd] Stop exposing Futures from ClangdServer operations.

sammccall created this revision. sammccall added a reviewer: ilya-biryukov. Herald added subscribers: cfe-commits, ioeric, jkorous-apple, klimek. LSP has asynchronous semantics, being able to block on an async operation completing is unneccesary and leads to tighter coupling with the threading. I

[PATCH] D43127: [clangd] Stop exposing Futures from ClangdServer operations.

sammccall updated this revision to Diff 133609. sammccall added a comment. Tidy up comment, and revert notify_all to notify_one - it was a red herring. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D43127 Files: clangd/ClangdServer.cpp clangd/ClangdServer.h clangd/TUSched

[PATCH] D42942: [clangd] Collect definitions when indexing.

sammccall marked an inline comment as done. sammccall added inline comments. Comment at: clangd/index/SymbolCollector.cpp:210 + BasicSymbol = addDeclaration(*ND, std::move(ID)); +if (Roles & static_cast(index::SymbolRole::Definition)) + addDefinition(*cast(ASTNode.O

[PATCH] D42942: [clangd] Collect definitions when indexing.

This revision was automatically updated to reflect the committed changes. Closed by commit rL324735: [clangd] Collect definitions when indexing. (authored by sammccall, committed by ). Herald added a subscriber: llvm-commits. Changed prior to commit: https://reviews.llvm.org/D42942?vs=133502&id

[PATCH] D42942: [clangd] Collect definitions when indexing.

This revision was automatically updated to reflect the committed changes. Closed by commit rCTE324735: [clangd] Collect definitions when indexing. (authored by sammccall, committed by ). Changed prior to commit: https://reviews.llvm.org/D42942?vs=133502&id=133613#toc Repository: rL LLVM htt

[PATCH] D43127: [clangd] Stop exposing Futures from ClangdServer operations.

sammccall updated this revision to Diff 133615. sammccall added a comment. rebase Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D43127 Files: clangd/ClangdServer.cpp clangd/ClangdServer.h clangd/TUScheduler.cpp clangd/TUScheduler.h clangd/Threading.cpp clangd/Thread

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

sammccall added a comment. Insertion side LGTM, feel free to split and land. Sorry I need to take off and will need to get to indexing on monday :( Comment at: clangd/ClangdServer.cpp:368 +/// Calculates the shortest possible include path when inserting \p Header to \p +///

[PATCH] D43127: [clangd] Stop exposing Futures from ClangdServer operations.

sammccall updated this revision to Diff 133857. sammccall marked 3 inline comments as done. sammccall added a comment. Change AsyncTaskRunner::wait() to be LLVM_NODISCARD when used with a deadline. Restore NoConcurrentDiagnostics test to its former glory (with comments) Other comment fixes. Repo

[PATCH] D43127: [clangd] Stop exposing Futures from ClangdServer operations.

sammccall added inline comments. Comment at: clangd/Threading.h:60 +/// A point in time we may wait for, or None to wait forever. +/// (We use Optional because buggy implementations of std::chrono overflow...) +using Deadline = llvm::Optional; ilya-biryukov wrote

[PATCH] D43182: [clangd] SymbolLocation only covers symbol name.

sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land. Nice! Comment at: clangd/index/Index.cpp:40 raw_ostream &operator<<(raw_ostream &OS, const Symbol &S) { - return OS << S.Scope << S.Name; + return OS << S.Scope << S

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

sammccall added a comment. Insertion still LG (couple of nits, inline). For indexing, my biggest questions: - I worry CanonicalIncludes doesn't get enough information to make good decisions - passing the include stack in some form may be better - CanonicalIncludes has slightly weird patterns of

[PATCH] D43127: [clangd] Stop exposing Futures from ClangdServer operations.

sammccall updated this revision to Diff 133868. sammccall marked 3 inline comments as done. sammccall added a comment. Review comments Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D43127 Files: clangd/ClangdServer.cpp clangd/ClangdServer.h clangd/TUScheduler.cpp clangd

[PATCH] D43127: [clangd] Stop exposing Futures from ClangdServer operations.

sammccall added inline comments. Comment at: clangd/TUScheduler.cpp:286 } // unlock Mutex. RequestsCV.notify_one(); } ilya-biryukov wrote: > Change to `notify_all()`? Otherwise we might wake up some thread waiting on > `blockUntilIdle()`, but not the work

[PATCH] D43127: [clangd] Stop exposing Futures from ClangdServer operations.

This revision was automatically updated to reflect the committed changes. Closed by commit rL324990: [clangd] Stop exposing Futures from ClangdServer operations. (authored by sammccall, committed by ). Herald added a subscriber: llvm-commits. Repository: rL LLVM https://reviews.llvm.org/D43127

[PATCH] D43230: [clangd] Explicitly initialize all primitive fields in Protocol.h

sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land. Yup, I got bitten recently from some of our plain-c-style structs with no default initializers (in Index). Definitely a fan of this change. Main downside is you can't use aggregate init

[PATCH] D43230: [clangd] Explicitly initialize all primitive fields in Protocol.h

sammccall added inline comments. Comment at: clangd/Protocol.h:80 struct Position { + Position() = default; + Position(int line, int character) : line(line), character(character) {} I'd lean to making callers initialize field-by-field here rather than provide

[PATCH] D43227: [clangd] Make functions of ClangdServer callback-based

sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land. LG, suggest a tweak to capture() though. I wonder whether we want to introduce `using Callback = UniqueFunction` for readability at some point... Comment at: clangd/C

[PATCH] D43246: [clangd] Assert path is absolute when assigning to URIForFile.

sammccall added inline comments. Comment at: clangd/Protocol.h:52 struct URIForFile { + URIForFile() = default; I don't like how the API changes here take us further away from the other structs in this file. Why does this one enforce invariants, when the oth

[PATCH] D43272: [clangd] Fix tracing now that spans lifetimes can overlap on a thread.

sammccall created this revision. sammccall added a reviewer: ioeric. Herald added subscribers: cfe-commits, jkorous-apple, ilya-biryukov, klimek. The chrome trace viewer requires events within a thread to strictly nest. So we need to record the lifetime of the Span objects, not the contexts. But

[PATCH] D43272: [clangd] Fix tracing now that spans lifetimes can overlap on a thread.

sammccall updated this revision to Diff 134386. sammccall added a comment. address review comments Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D43272 Files: clangd/Trace.cpp clangd/Trace.h test/clangd/trace.test unittests/clangd/TraceTests.cpp Index: unittests/clangd

[PATCH] D43272: [clangd] Fix tracing now that spans lifetimes can overlap on a thread.

sammccall added inline comments. Comment at: clangd/Trace.cpp:133 +std::atomic EndTime; // Filled in by endSpan(). +std::string Name; +const uint64_t TID; ilya-biryukov wrote: > Maybe make all fields except `EndTime` const? Good idea - what's safe to

[PATCH] D43272: [clangd] Fix tracing now that spans lifetimes can overlap on a thread.

This revision was automatically updated to reflect the committed changes. Closed by commit rL325220: [clangd] Fix tracing now that spans lifetimes can overlap on a thread. (authored by sammccall, committed by ). Herald added a subscriber: llvm-commits. Repository: rL LLVM https://reviews.llvm.

[PATCH] D43229: [clangd] Enable snippet completion based on client capabilities.

sammccall added inline comments. Comment at: clangd/tool/ClangdMain.cpp:60 -static llvm::cl::opt EnableSnippets( -"enable-snippets", ilya-biryukov wrote: > ioeric wrote: > > Would we still have a way to disable snippets (e.g. for debugging) even if > > cli

[PATCH] D43272: [clangd] Fix tracing now that spans lifetimes can overlap on a thread.

sammccall added a comment. r325239 should fix, sorry! Repository: rL LLVM https://reviews.llvm.org/D43272 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43246: [clangd] Assert path is absolute when assigning to URIForFile.

sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land. LG if we want to do this (please getFile -> file though!) Comment at: clangd/Protocol.h:52 struct URIForFile { + URIForFile() = default; ioeric wro

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

sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land. LG apart from the .inc handling (happy to chat more) Comment at: clangd/index/CanonicalIncludes.h:52 + /// a canonical header name. + llvm::StringRef mapHeader(llvm::

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

sammccall added inline comments. Comment at: unittests/clangd/HeadersTests.cpp:29 + // absolute path. + std::string addToSubdir(PathRef File, llvm::StringRef Code = "") { +assert(llvm::sys::path::is_relative(File) && "FileName should be relative"); sammcca

[PATCH] D43377: [clangd] Attach more information about Sema completion to traces

sammccall added a comment. This looks really useful! Main suggestion is to drop the added span and attach kind to the main span instead. (It's relevant to index too, not just to sema) Comment at: clangd/CodeComplete.cpp:403 +StringRef printCompletionKind(enum CodeCompletionC

[PATCH] D43381: [clangd] Fix use-after-free in SymbolYAML: strings are owned by yaml::Input!

sammccall created this revision. sammccall added a reviewer: ioeric. Herald added subscribers: cfe-commits, jkorous-apple, ilya-biryukov, klimek. There are a few implementation options here - alternatives are either both awkward and inefficient, or really inefficient. This is at least potentially

[PATCH] D43385: [clangd] Add "clangd.trace" VSCode setting to enable tracing.

sammccall created this revision. sammccall added a reviewer: ilya-biryukov. Herald added subscribers: cfe-commits, ioeric, jkorous-apple, klimek. Setting the CLANGD_TRACE environment variable directly is awkward with VSCode's "reload from the command palette" workflow. Repository: rCTE Clang T

[PATCH] D43388: [clangd] Tracing: name worker threads, and enforce naming scheduled async tasks

sammccall created this revision. sammccall added a reviewer: ilya-biryukov. Herald added subscribers: cfe-commits, ioeric, jkorous-apple, klimek. This has a bit of a blast radius, but I think there's enough value in "forcing" us to give names to these async tasks for debugging. Guessing about what

<    1   2   3   4   5   6   7   8   9   10   >