[PATCH] D51527: [NFC] Use LLVM naming conventions within FileDistance

sammccall accepted this revision. sammccall added inline comments. This revision is now accepted and ready to land. Comment at: clang-tools-extra/clangd/FileDistance.cpp:56 -constexpr const unsigned FileDistance::kUnreachable; - I think this is required - are y

[PATCH] D51279: [clangd] Implement findOccurrences interface in dynamic index.

sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land. This basically looks good to go (some fixes needed but they're pretty clear I think let me know if not!) Comment at: clangd/index/FileIndex.cpp:63 + auto Occurrences

[PATCH] D51291: [clangd] Support multiple #include headers in one symbol.

sammccall accepted this revision. sammccall added inline comments. This revision is now accepted and ready to land. Comment at: clangd/CodeComplete.cpp:396 +// Prefer includes that do not need edits (i.e. already exist). +std::stable_sort(Completion.Includes.begin(), Comp

[PATCH] D51422: [clangd] Factor out the data-swapping functionality from MemIndex/DexIndex.

sammccall added inline comments. Comment at: clangd/index/Index.h:416 + // until the call returns (even if reset() is called). + bool fuzzyFind(const FuzzyFindRequest &, + llvm::function_ref) const override; kbobyrev wrote: > Do we want these fu

[PATCH] D51539: [clangd] Add symbol slab size to index memory consumption estimates

sammccall added a comment. This looks reasonable! This is going to conflict with https://reviews.llvm.org/D51422, you might want to rebase. Comment at: clang-tools-extra/clangd/index/MemIndex.h:26 + void build(std::shared_ptr> Symbols, + size_t SlabSize=0);

[PATCH] D51504: [clangd] Flatten out Symbol::Details. It was ill-conceived, sorry.

This revision was automatically updated to reflect the committed changes. Closed by commit rL341211: [clangd] Flatten out Symbol::Details. It was ill-conceived, sorry. (authored by sammccall, committed by ). Herald added a subscriber: llvm-commits. Repository: rL LLVM https://reviews.llvm.org/

[PATCH] D51279: [clangd] Implement findOccurrences interface in dynamic index.

sammccall added inline comments. Comment at: unittests/clangd/TestTU.h:41 + static TestTU withAllCode(llvm::StringRef HeaderCode, llvm::StringRef Code, +llvm::StringRef Filename = "") { hokein wrote: > sammccall wrote: > > We've avo

[PATCH] D51422: [clangd] Factor out the data-swapping functionality from MemIndex/DexIndex.

sammccall added inline comments. Comment at: clangd/index/FileIndex.h:47 + // The shared_ptr keeps the symbols alive. + std::shared_ptr buildMemIndex(); ioeric wrote: > Maybe avoid hardcoding the index name, so that we could potentially switch to > use a dif

[PATCH] D51585: [clangd] Define a compact binary serialization fomat for symbol slab/index.

sammccall created this revision. sammccall added a reviewer: ioeric. Herald added subscribers: cfe-commits, kadircet, arphaman, mgrang, jkorous, MaskRay, ilya-biryukov, mgorny. This is intended to replace the current YAML format for general use. It's ~10x more compact than YAML, and ~40% more com

[PATCH] D51585: [clangd] Define a compact binary serialization fomat for symbol slab/index.

sammccall updated this revision to Diff 163688. sammccall added a comment. Revert unused changes. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D51585 Files: clangd/CMakeLists.txt clangd/RIFF.cpp clangd/RIFF.h clangd/global-symbol-builder/GlobalSymbolBuilderMain.cpp c

[PATCH] D51585: [clangd] Define a compact binary serialization fomat for symbol slab/index.

sammccall added a comment. This still needs to be synced to head to account for Symbol changes (multi-includes) but those changes are pretty obvious/mechanical. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D51585 ___ cfe-commits m

[PATCH] D51422: [clangd] Factor out the data-swapping functionality from MemIndex/DexIndex.

sammccall updated this revision to Diff 163714. sammccall added a comment. Instead of returning shared_ptr, our implementations that have backing data gain the ability to own that data (without coupling to its form). Based on offline discussion with @ioeric. Rebase to pick up SymbolOccurrence cha

[PATCH] D51422: [clangd] Factor out the data-swapping functionality from MemIndex/DexIndex.

sammccall updated this revision to Diff 163715. sammccall added a comment. Remove some more shared_ptr occurrences that aren't needed anymore Update comments Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D51422 Files: clangd/index/FileIndex.cpp clangd/index/FileIndex.h cl

[PATCH] D51422: [clangd] Factor out the data-swapping functionality from MemIndex/DexIndex.

sammccall added a comment. OK, I think this is good to go again. (A couple of seemingly-unrelated fixes to SymbolOccurrenceKind that I ran into in tests) Comment at: clangd/index/FileIndex.h:47 + // The shared_ptr keeps the symbols alive. + std::shared_ptr buildMemIndex();

[PATCH] D51422: [clangd] Factor out the data-swapping functionality from MemIndex/DexIndex.

This revision was automatically updated to reflect the committed changes. Closed by commit rL341318: [clangd] Factor out the data-swapping functionality from MemIndex/DexIndex. (authored by sammccall, committed by ). Herald added a subscriber: llvm-commits. Changed prior to commit: https://revi

[PATCH] D51585: [clangd] Define a compact binary serialization fomat for symbol slab/index.

sammccall updated this revision to Diff 163722. sammccall added a comment. Rebase with occurrences changes (but don't serialize occurrences yet). Also incorporate multiple-include-header change, which is tricky as it makes Symbol variable-length. Current hack is to preserve only the most-popular 5

[PATCH] D50958: [clangd] Implement findReferences function

sammccall added a comment. Looking forward to using this! Unless you object, I'd like to address the remaining comments (and get a review), so you can make the most of your leave! Comment at: clangd/XRefs.cpp:333 + +DeclOccurrences[D].emplace_back(FileLoc, Roles); +re

[PATCH] D51321: [Tooling] Improve handling of CL-style options

sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land. Thanks again for fixing this. Still a few places I feel the code could be simpler, but will let you decide how to deal with them. I would greatly appreciate removing the parameterized tes

[PATCH] D51221: [clangd] Some nitpicking around the new split (preamble/main) dynamic index

sammccall added inline comments. Comment at: clangd/ClangdServer.cpp:152 WorkScheduler(Opts.AsyncThreadsCount, Opts.StorePreamblesInMemory, -DynamicIdx ? *DynamicIdx : noopParsingCallbacks(), +DynamicIdx ? DynamicIdx->makeUpdateCallb

[PATCH] D51221: [clangd] Some nitpicking around the new split (preamble/main) dynamic index

sammccall updated this revision to Diff 163730. sammccall marked 2 inline comments as done. sammccall added a comment. Address review comments Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D51221 Files: clangd/ClangdServer.cpp clangd/ClangdServer.h clangd/ClangdUnit.cpp

[PATCH] D51221: [clangd] Some nitpicking around the new split (preamble/main) dynamic index

This revision was automatically updated to reflect the committed changes. Closed by commit rL341325: [clangd] Some nitpicking around the new split (preamble/main) dynamic index (authored by sammccall, committed by ). Herald added a subscriber: llvm-commits. Changed prior to commit: https://revi

[PATCH] D51585: [clangd] Define a compact binary serialization fomat for symbol slab/index.

sammccall updated this revision to Diff 163743. sammccall added a comment. Fix subtle macro expansion bug! Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D51585 Files: clangd/CMakeLists.txt clangd/RIFF.cpp clangd/RIFF.h clangd/global-symbol-builder/GlobalSymbolBuilderMai

[PATCH] D51585: [clangd] Define a compact binary serialization fomat for symbol slab/index.

sammccall updated this revision to Diff 163744. sammccall added a comment. Fix comment Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D51585 Files: clangd/CMakeLists.txt clangd/RIFF.cpp clangd/RIFF.h clangd/global-symbol-builder/GlobalSymbolBuilderMain.cpp clangd/index

[PATCH] D51605: [clangd] SymbolOccurrences -> Refs and cleanup

sammccall created this revision. sammccall added a reviewer: ioeric. Herald added subscribers: cfe-commits, kadircet, arphaman, mgrang, jkorous, MaskRay, ilya-biryukov. A few things that I noticed while merging the SwapIndex patch: - SymbolOccurrences and particularly SymbolOccurrenceSlab are un

[PATCH] D51605: [clangd] SymbolOccurrences -> Refs and cleanup

sammccall updated this revision to Diff 163751. sammccall added a comment. Remove RefsSlab::find() Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D51605 Files: clangd/ClangdServer.cpp clangd/index/FileIndex.cpp clangd/index/FileIndex.h clangd/index/Index.cpp clangd/ind

[PATCH] D51605: [clangd] SymbolOccurrences -> Refs and cleanup

sammccall marked 7 inline comments as done. sammccall added a comment. In https://reviews.llvm.org/D51605#1222636, @hokein wrote: > Thanks for cleaning it up! I admit that `SymbolOccurrences` is a loong name. > A few nits. Yeah, I hope you don't mind! The references stuff is awesome, I'm hopin

[PATCH] D51605: [clangd] SymbolOccurrences -> Refs and cleanup

sammccall updated this revision to Diff 163784. sammccall marked 4 inline comments as done. sammccall added a comment. Address comments Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D51605 Files: clangd/ClangdServer.cpp clangd/index/FileIndex.cpp clangd/index/FileIndex.h

[PATCH] D51605: [clangd] SymbolOccurrences -> Refs and cleanup

sammccall marked an inline comment as done. sammccall added inline comments. Comment at: clangd/index/Index.cpp:158 +auto &SymRefs = Sym.second; +std::sort(SymRefs.begin(), SymRefs.end()); +// TODO: do we really need to dedup? lebedev.ri wrote: > I no

[PATCH] D51605: [clangd] SymbolOccurrences -> Refs and cleanup

This revision was automatically updated to reflect the committed changes. sammccall marked an inline comment as done. Closed by commit rCTE341368: [clangd] SymbolOccurrences -> Refs and cleanup (authored by sammccall, committed by ). Changed prior to commit: https://reviews.llvm.org/D51605?vs=1

[PATCH] D51626: [clangd] Move buildStaticIndex() to SymbolYAML

sammccall accepted this revision. sammccall added inline comments. This revision is now accepted and ready to land. Comment at: clang-tools-extra/clangd/index/SymbolYAML.cpp:193 + auto Slab = symbolsFromYAML(Buffer.get()->getBuffer()); + SymbolSlab::Builder SymsBuilder; + for

[PATCH] D50958: [clangd] Implement findReferences function

sammccall added a comment. OK thanks, I'll steal this one. Comment at: clangd/XRefs.cpp:724 + // traversing the AST, and we don't want to make unnecessary queries to the + // index. Howerver, we don't have a reliable way to distinguish file-local + // symbols. We conservativ

[PATCH] D51636: [clangd] NFC: Change quality type to float

sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land. If it's not too expensive, we can use the symbol quality metrics (from Quality.h) to do a more accurate prescore. Worth a benchmark :-) https://reviews.llvm.org/D51636 __

[PATCH] D50958: [clangd] Implement findReferences function

sammccall updated this revision to Diff 163834. sammccall added a comment. Address comments and tighten up highlights/refs reuse in XRefs.cpp a bit. Still to do: - test interaction with index, including actual data - maybe add the ClangdServer/LSP binding and lit test, if it's just boilerplate

[PATCH] D50958: [clangd] Implement findReferences function

sammccall added a comment. Oops, and I rebased on head (Occurrences -> Refs) of course. @ilya-biryukov @ioeric, any interest in taking over review here? Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D50958 ___ cfe-commits mailing l

[PATCH] D51585: [clangd] Define a compact binary serialization fomat for symbol slab/index.

sammccall updated this revision to Diff 163836. sammccall marked an inline comment as done. sammccall added a comment. address review comments Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D51585 Files: clangd/CMakeLists.txt clangd/RIFF.cpp clangd/RIFF.h clangd/global-s

[PATCH] D51585: [clangd] Define a compact binary serialization fomat for symbol slab/index.

sammccall marked 2 inline comments as done. sammccall added inline comments. Comment at: clangd/index/Serialization.cpp:50 + +void writeVar(uint32_t I, raw_ostream &OS) { + constexpr static uint8_t More = 1 << 7; ioeric wrote: > This function could use a comment

[PATCH] D51036: clang-format: Fix formatting C++ namespaces with preceding 'inline' or 'export' specifier

sammccall added a comment. In https://reviews.llvm.org/D51036#1223230, @melver wrote: > Awaiting remaining reviewer acceptance. > > FYI: I do not have commit commit access -- what is the procedure to commit > once diff is accepted? > > Many thanks! Anyone with commit access can land it for you

[PATCH] D51638: [clangd] Load static index asynchronously, add tracing.

sammccall created this revision. sammccall added a reviewer: ioeric. Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, MaskRay, ilya-biryukov. Like https://reviews.llvm.org/D51475 but simplified based on recent patches. While here, clarify that loadIndex() takes a filename, not

[PATCH] D51585: [clangd] Define a compact binary serialization fomat for symbol slab/index.

This revision was automatically updated to reflect the committed changes. Closed by commit rL341375: [clangd] Define a compact binary serialization fomat for symbol slab/index. (authored by sammccall, committed by ). Herald added a subscriber: llvm-commits. Changed prior to commit: https://revi

[PATCH] D51638: [clangd] Load static index asynchronously, add tracing.

This revision was automatically updated to reflect the committed changes. Closed by commit rL341376: [clangd] Load static index asynchronously, add tracing. (authored by sammccall, committed by ). Herald added a subscriber: llvm-commits. Changed prior to commit: https://reviews.llvm.org/D51638?

[PATCH] D51641: [VFS] Cache the current working directory for the real FS.

sammccall added inline comments. Comment at: lib/Basic/VirtualFileSystem.cpp:275 return EC; - return Dir.str().str(); + CWDCache = Dir.str(); + return CWDCache; Doesn't this need to be guarded by a lock? I know the current version is thread-hostile in pr

[PATCH] D51638: [clangd] Load static index asynchronously, add tracing.

sammccall added a comment. I think you're right - this isn't actually asynchronous (I didn't manage to test that!), and if it were it'd interfere with clean shutdown. I think both issues can be addressed by assigning the future to a variable scoped to main. Will send a patch. Repository: rL

[PATCH] D51036: clang-format: Fix formatting C++ namespaces with preceding 'inline' or 'export' specifier

This revision was automatically updated to reflect the committed changes. Closed by commit rL341450: clang-format: Fix formatting C++ namespaces with preceding 'inline' or 'export'… (authored by sammccall, committed by ). Herald added a subscriber: llvm-commits. Repository: rL LLVM https://re

[PATCH] D51674: [clangd] Fix async index loading (from r341376).

sammccall created this revision. sammccall added a reviewer: ilya-biryukov. Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, MaskRay, ioeric. This wasn't actually async (due to std::future destructor blocking). If it were, we would have clean shutdown issues if main returned an

[PATCH] D51641: [VFS] Cache the current working directory for the real FS.

sammccall accepted this revision. sammccall added inline comments. This revision is now accepted and ready to land. Comment at: lib/Basic/VirtualFileSystem.cpp:248 +private: + // Make sure `CWDCache` update is thread safe in `getCurrentWorkingDirectory`. + mutable std::mutex M

[PATCH] D50958: [clangd] Implement findReferences function

sammccall updated this revision to Diff 164004. sammccall added a comment. Finish tests, fix TestTU::index() to include occurrences. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D50958 Files: clangd/XRefs.cpp clangd/XRefs.h unittests/clangd/TestTU.cpp unittests/clangd/

[PATCH] D50958: [clangd] Implement findReferences function

sammccall added inline comments. Comment at: clangd/XRefs.cpp:328 + : AST(AST) { +for (const Decl *D : TargetDecls) + Targets.insert(D); hokein wrote: > nit: we can initialize TargetDecls like `Targets(TargetDecls.begin(), > TargetDecls.end())`. In

[PATCH] D50958: [clangd] Implement findReferences function

sammccall closed this revision. sammccall added a comment. Oops, I forgot to associate my patch with this review. It landed as https://reviews.llvm.org/rL341458. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D50958 ___ cfe-commits

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

sammccall commandeered this revision. sammccall added a reviewer: hokein. sammccall added a comment. Herald added a subscriber: kadircet. (Taking this as @hokein is on leave and the dependency has landed in https://reviews.llvm.org/D50958) Repository: rCTE Clang Tools Extra https://reviews.l

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

sammccall updated this revision to Diff 164010. sammccall added a comment. Updated based on findReferences implementation which has now landed. Removed ReferenceContext support for now, implementation has no options. references->findReferences in ClangdServer (more consistent with other methods) A

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

sammccall marked 4 inline comments as done. sammccall added a comment. PTAL Comment at: clangd/ClangdServer.h:158 + /// Retrieve locations for symbol references. + void references(PathRef File, Position Pos, bool includeDeclaration, + Callback> CB); -

[PATCH] D51674: [clangd] Fix async index loading (from r341376).

sammccall updated this revision to Diff 164012. sammccall added a comment. Don't load index asynchronously if -run-synchronously is passed. Nothing needs this today, but it's less surprising behavior. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D51674 Files: clangd/tool/Cl

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

This revision was automatically updated to reflect the committed changes. sammccall marked 3 inline comments as done. Closed by commit rCTE341462: [clangd] Add xrefs LSP boilerplate implementation. (authored by sammccall, committed by ). Changed prior to commit: https://reviews.llvm.org/D50896?

[PATCH] D51675: [Sema] Store MacroInfo in CodeCompletionResult for macro results.

sammccall accepted this revision. sammccall added inline comments. This revision is now accepted and ready to land. Comment at: include/clang/Sema/CodeCompleteConsumer.h:847 + /// If the result is RK_Macro, this can store the information about the macro + /// definition.

[PATCH] D51689: [clangd] Dense posting lists proof-of-concept

sammccall created this revision. sammccall added a reviewer: kbobyrev. Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, MaskRay, ioeric, ilya-biryukov. This uses a bitmap representation instead of a list if the density of the list is high enough (at least 1 in 32, which is the

[PATCH] D51688: [clangd] Set SymbolID for sema macros so that they can be merged with index macros.

sammccall accepted this revision. sammccall added inline comments. This revision is now accepted and ready to land. Comment at: clangd/AST.h:41 +/// Gets the symbol ID for a macro, if possible. +llvm::Optional getSymbolID(const IdentifierInfo &II, +

[PATCH] D51691: [clangd] NFC: Document URIDistance

sammccall accepted this revision. sammccall added inline comments. This revision is now accepted and ready to land. Comment at: clang-tools-extra/clangd/FileDistance.h:66 // Supports lookups to find the minimum distance to a file from any source. -// This object should be reused

[PATCH] D51676: [clangd] Use TopN instead of std::priority_queue

sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land. Thanks for cleaning this up! I believe this will result in the results from MemIndex being returned in best -> worst order, rather than worst -> best. The contract says callers shouldn't

[PATCH] D51724: [clangd] Add "Deprecated" field to Symbol and CodeCompletion.

sammccall added a comment. I think you also need to update SymbolsYAML and Serialization. Comment at: clangd/Protocol.cpp:520 Result["additionalTextEdits"] = json::Array(CI.additionalTextEdits); + if (CI.deprecated) +Result["deprecated"] = CI.deprecated; -

[PATCH] D51689: [clangd] Dense posting lists proof-of-concept

sammccall updated this revision to Diff 164191. sammccall added a comment. [Tooling] JSONCompilationDatabasePlugin infers compile commands for missing files See the existing InterpolatingCompilationDatabase for details on how this works. We've been using this in clangd for a while, the heuristic

[PATCH] D51689: [clangd] Dense posting lists proof-of-concept

sammccall added a comment. Uh, please ignore the last comment, arc/I got confused. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D51689 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/li

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

sammccall created this revision. sammccall added a reviewer: bkramer. Herald added subscribers: cfe-commits, kadircet, ioeric, ilya-biryukov. See the existing InterpolatingCompilationDatabase for details on how this works. We've been using this in clangd for a while, the heuristics seem to work we

[PATCH] D45045: [DebugInfo] Generate debug information for labels.

sammccall added a comment. The latest reland of this patch triggers a segfault in clang (seemingly only on k8 and ppc). I've bisected it to r341519 but don't understand the cause. The stacktrace is: 1. parser at end of file 2.Code generation #0 0x5649754494d4 SignalHandler(int)

[PATCH] D51724: [clangd] Add "Deprecated" field to Symbol and CodeCompletion.

sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land. Agree with Kadir's comments. I'd just suggest reducing boilerplate a bit by taking some shortcuts. Comment at: clangd/index/Index.h:167 +enum class SymbolFlag : uint8

[PATCH] D51539: [clangd] Add symbol slab size to index memory consumption estimates

sammccall added a comment. LG with a few nits Comment at: clang-tools-extra/clangd/index/FileIndex.cpp:121 +size_t FileSymbols::estimateMemoryUsage() const { + size_t Result = 0; + for (const auto &FileAndSlab : FileToSlabs) this isn't safe as it doesn't acqu

[PATCH] D51774: [clangd] NFC: Rename DexIndex to Dex

sammccall accepted this revision. sammccall added inline comments. This revision is now accepted and ready to land. Comment at: clang-tools-extra/clangd/index/dex/Dex.h:20 #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_INDEX_DEX_DEXINDEX_H #define LLVM_CLANG_TOOLS_EXTRA_CLANGD_INDEX_D

[PATCH] D51539: [clangd] Add symbol slab size to index memory consumption estimates

sammccall added inline comments. Comment at: clang-tools-extra/clangd/index/FileIndex.cpp:135 + std::move(RefsStorage)), + /*BackingDataSize=*/0); } this size should be calculated from the slabs above https://reviews.llvm.org/D51539

[PATCH] D51539: [clangd] Add symbol slab size to index memory consumption estimates

sammccall added inline comments. Comment at: clang-tools-extra/clangd/index/FileIndex.cpp:131 + size_t StorageSize = 0; + for (const auto &Slab : SymbolSlabs) +StorageSize += Slab->bytes(); also the refslabs and refsstorage https://reviews.llvm.org/D51539

[PATCH] D51628: [clangd] Implement a Proof-of-Concept tool for symbol index exploration

sammccall added a comment. A tool like this will be a useful addition! I think the big high-level questions are: - UI: a REPL seems like a good model, but we need a more user-friendly way to read commands - Testing: need a plan for either a) testing the commands or b) keeping the commands simp

[PATCH] D51674: [clangd] Fix async index loading (from r341376).

This revision was automatically updated to reflect the committed changes. Closed by commit rL341797: [clangd] Fix async index loading (from r341376). (authored by sammccall, committed by ). Herald added a subscriber: llvm-commits. Changed prior to commit: https://reviews.llvm.org/D51674?vs=1640

[PATCH] D51852: [clangd] Implement FuzzyFindRequest JSON (de)serialization

sammccall accepted this revision. sammccall added inline comments. This revision is now accepted and ready to land. Comment at: clang-tools-extra/clangd/CodeComplete.cpp:1384 Req.ProximityPaths.push_back(FileName); -vlog("Code complete: fuzzyFind(\"{0}\", scopes=[{1}])",

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

sammccall added a comment. In https://reviews.llvm.org/D51747#1227921, @ioeric wrote: > In https://reviews.llvm.org/D51747#1227089, @ilya-biryukov wrote: > > > > ! In https://reviews.llvm.org/D51747#1227719, @kadircet wrote: > > > > > >> ! In https://reviews.llvm.org/D51747#1227089, @ilya-biryuk

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

sammccall added inline comments. Comment at: clangd/ClangdServer.cpp:535 // Inject the resource dir. // FIXME: Don't overwrite it if it's already there. C->CommandLine.push_back("-resource-dir=" + ResourceDir); can you add a comment that these flags work

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

sammccall added a comment. In https://reviews.llvm.org/D51747#1228919, @ilya-biryukov wrote: > > Most of the value of adding an option is: if someone complains, we can tell > > them to go away :-) One possible corollary is: we shouldn't add the option > > until someone complains. I'm not 100% s

[PATCH] D51860: [clangd] NFC: Use uint32_t for FuzzyFindRequest limits

sammccall added a comment. Why this change? (If you're changing it, `Optional` is probably clearer) https://reviews.llvm.org/D51860 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51860: [clangd] NFC: Use uint32_t for FuzzyFindRequest limits

sammccall added inline comments. Comment at: clang-tools-extra/clangd/index/Index.h:440 /// return more than this, e.g. if it doesn't know which candidates are best. - size_t MaxCandidateCount = std::numeric_limits::max(); + uint32_t MaxCandidateCount = std::numeric_limits::

[PATCH] D51090: [clangd] Add index benchmarks

sammccall added a comment. Nice and simple :-) Looks good, just some details. Comment at: clang-tools-extra/clangd/benchmarks/IndexBenchmark.cpp:1 +//===--- DexBenchmark.cpp - DexIndex benchmarks -*- C++ -*-===// +// rename? (it's not just dex

[PATCH] D51214: [clangd] Add options to enable/disable fixits and function argument snippets.

sammccall added inline comments. Comment at: clangd/tool/ClangdMain.cpp:197 +static llvm::cl::opt IncludeFixIts( +"include-fixits", ilya-biryukov wrote: > sammccall wrote: > > ilya-biryukov wrote: > > > I wonder if we should make the `IncludeFixIts` option

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

sammccall created this revision. sammccall added a reviewer: bkramer. Herald added subscribers: cfe-commits, fedor.sergeev. Most callers I can find are using only `getName()`. Type is used by the recursive iterator. Now we don't have to call stat() on every listed file (on most platforms). Except

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

sammccall added inline comments. Comment at: include/clang/Basic/VirtualFileSystem.h:135 + // For compatibility with old Status-based API. Prefer using Path directly. + StringRef getName() const { return Path; } +}; Backwards-compatibility notes: - Almost all

[PATCH] D51628: [clangd] Implement a Proof-of-Concept tool for symbol index exploration

sammccall added inline comments. Comment at: clang-tools-extra/clangd/index/dex/dexplorer/DExplorer.cpp:23 +#include "llvm/Support/Signals.h" +#include "llvm/Support/YAMLTraits.h" +#include why? Comment at: clang-tools-extra/clangd/index/dex/d

[PATCH] D51214: [clangd] Add options to enable/disable fixits and function argument snippets.

sammccall added a comment. I think I'm still where I was a few weeks ago - option to drop args makes sense, completions with fixes isn't something users should care about. Comment at: clangd/tool/ClangdMain.cpp:197 +static llvm::cl::opt IncludeFixIts( +"include-fixits",

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

sammccall added a comment. In https://reviews.llvm.org/D51747#1230420, @kadircet wrote: > In https://reviews.llvm.org/D51747#1229066, @sammccall wrote: > > > In https://reviews.llvm.org/D51747#1228919, @ilya-biryukov wrote: > > > > > > Most of the value of adding an option is: if someone complain

[PATCH] D51628: [clangd] Implement a Proof-of-Concept tool for symbol index exploration

sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land. Really just details now. Comment at: clang-tools-extra/clangd/index/dex/dexp/Dexp.cpp:56 +// llvm::formatv string pattern for pretty-printing symbols. +static const aut

[PATCH] D51090: [clangd] Add index benchmarks

sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land. In https://reviews.llvm.org/D51090#1230582, @lebedev.ri wrote: > In https://reviews.llvm.org/D51090#1230579, @kbobyrev wrote: > > > The only problem left is that I'm not sure how to run b

[PATCH] D51989: [clangd] dexp tool uses llvm::cl to parse its flags.

sammccall created this revision. sammccall added a reviewer: kbobyrev. Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, MaskRay, ioeric, ilya-biryukov. We can use cl::ResetCommandLineParser() to support different types of command-lines, as long as we're careful about option lif

[PATCH] D51987: [clangd] Rename global-symbol-builder to clangd-symbol-builder.

sammccall accepted this revision. sammccall added a comment. Much better. I think `clangd-indexer` might be **even** better, but up to you. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D51987 ___ cfe-commits mailing list cfe-commit

[PATCH] D51987: [clangd] Rename global-symbol-builder to clangd-symbol-builder.

sammccall added a comment. In https://reviews.llvm.org/D51987#1231993, @ioeric wrote: > In https://reviews.llvm.org/D51987#1231990, @ilya-biryukov wrote: > > > > You beat it to me, but I thought we didn't use indexer as it could be > > > confused with the actual indexing? > > > > Sorry. Not a bi

[PATCH] D51996: [clangd] Simplify cancellation public API

sammccall created this revision. sammccall added reviewers: ilya-biryukov, kadircet. Herald added subscribers: cfe-commits, jfb, arphaman, jkorous, MaskRay, ioeric. Task is no longer exposed: - task cancellation is hidden as a std::function - task creation returns the new context directly - check

[PATCH] D51996: [clangd] Simplify cancellation public API

sammccall updated this revision to Diff 165125. sammccall added a comment. Fix includes, formatting tweak. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D51996 Files: clangd/Cancellation.cpp clangd/Cancellation.h clangd/ClangdLSPServer.cpp clangd/ClangdLSPServer.h cla

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

sammccall added inline comments. Comment at: include/clang/Basic/VirtualFileSystem.h:135 + // For compatibility with old Status-based API. Prefer using Path directly. + StringRef getName() const { return Path; } +}; bkramer wrote: > sammccall wrote: > > Backwar

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

sammccall created this revision. sammccall added reviewers: ilya-biryukov, kadircet. Herald added subscribers: cfe-commits, arphaman, jkorous, MaskRay, ioeric. The cancelable scopes are managed by JSONRPCDispatcher so that all Handlers run in cancelable contexts. (Previously ClangdServer did this,

[PATCH] D51996: [clangd] Simplify cancellation public API

sammccall added a comment. In https://reviews.llvm.org/D51996#1232459, @kadircet wrote: > Seems a lot cleaner now, thanks! > > Do you plan to have other changes like moving control to JSONRPCDispatcher > and recording timings for analysis on this patch? https://reviews.llvm.org/D52004 is the J

[PATCH] D51996: [clangd] Simplify cancellation public API

sammccall updated this revision to Diff 165158. sammccall marked 2 inline comments as done. sammccall added a comment. Address comments by adding comments! Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D51996 Files: clangd/Cancellation.cpp clangd/Cancellation.h clangd/Cla

[PATCH] D51333: Diagnose likely typos in include statements

sammccall added a comment. (No action needed, but curious on your thoughts) One thing I'd like to do sometime is add code completion of filenames in #include directives. This would mean listing the relevant directories from the include path. (VFS is slow for this today, but I have a patch out f

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

sammccall updated this revision to Diff 165250. sammccall added a comment. Path/getName() -> path(), Type -> type() for consistency with fs::directory_entry Repository: rC Clang https://reviews.llvm.org/D51921 Files: include/clang/Basic/VirtualFileSystem.h lib/Basic/VirtualFileSystem.cp

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

sammccall added inline comments. Comment at: include/clang/Basic/VirtualFileSystem.h:135 + // For compatibility with old Status-based API. Prefer using Path directly. + StringRef getName() const { return Path; } +}; sammccall wrote: > bkramer wrote: > > sammcca

[PATCH] D51996: [clangd] Simplify cancellation public API

sammccall added a comment. In https://reviews.llvm.org/D51996#1232770, @ilya-biryukov wrote: > > but the thing you're spending the CPU on is checking for cancellation... > > Unless checking for cancellation is really cheap (which is doable, I think). > We should probably hit some of those in pra

[PATCH] D51996: [clangd] Simplify cancellation public API

This revision was automatically updated to reflect the committed changes. Closed by commit rCTE342130: [clangd] Simplify cancellation public API (authored by sammccall, committed by ). Changed prior to commit: https://reviews.llvm.org/D51996?vs=165158&id=165251#toc Repository: rCTE Clang Too

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

sammccall updated this revision to Diff 165253. sammccall marked 2 inline comments as done. sammccall added a comment. rebase and address comments Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D52004 Files: clangd/ClangdLSPServer.cpp clangd/ClangdLSPServer.h clangd/Clangd

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