[PATCH] D67172: [clangd] Use pre-populated mappings for standard symbols

ilya-biryukov created this revision. ilya-biryukov added reviewers: sammccall, hokein. Herald added subscribers: jfb, kadircet, arphaman, jkorous, MaskRay. Herald added a project: clang. This takes ~5% of time when running clangd unit tests. Repository: rG LLVM Github Monorepo https://reviews

[PATCH] D67172: [clangd] Use pre-populated mappings for standard symbols

ilya-biryukov added a comment. Also considered moving the std symbol mapping into a separate (private-to-implementation) class, e.g. we don't use suffix mappings and symbol mappings outside system includes. But decided to wait until suffix mapping are going away completely. Repository: rG LL

[PATCH] D67096: [clangd][vscode] Add a flag to enable semantic highlighting in clangd

ilya-biryukov marked an inline comment as done. ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/clients/clangd-vscode/src/extension.ts:113 const semanticHighlightingFeature = - new semanticHighlighting.SemanticHighlightingFeature(); + new se

[PATCH] D67165: [clangd][vscode] Make SemanticHighlightingFeature more self-contained.

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts:88 +// recover from clangd crashes. +this.dispose(); +// Register the handler to handle semantic highlighting notification. C

[PATCH] D67165: [clangd][vscode] Make SemanticHighlightingFeature more self-contained.

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM, thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67165/new/ https://reviews.llvm.org/D67165

[PATCH] D67096: [clangd][vscode] Add a flag to enable semantic highlighting in clangd

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/clients/clangd-vscode/src/extension.ts:113 const semanticHighlightingFeature = - new semanticHighlighting.SemanticHighlightingFeature(); + new semanticHighlighting.SemanticHighlightingFeature(

[PATCH] D67096: [clangd][vscode] Add a flag to enable semantic highlighting in clangd

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM, thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67096/new/ https://reviews.llvm.org/D67096

[PATCH] D67224: [clangd] Enable completions with fixes in VSCode

ilya-biryukov created this revision. Herald added subscribers: cfe-commits, kadircet, arphaman, mgrang, jkorous, MaskRay. Herald added a project: clang. Currently, the completions are not shown to the user as VSCode tries to match 'filterText' against the text in the edit range. Since the text co

[PATCH] D67224: [clangd] Enable completions with fixes in VSCode

ilya-biryukov planned changes to this revision. ilya-biryukov added a comment. Changing behavior of clangd might affect other clients. It would be nice to find alternative ways to fix this. Nevertheless, posting this here as an example of a potential workaround for the problem. Repository: r

[PATCH] D67172: [clangd] Use pre-populated mappings for standard symbols

ilya-biryukov added a comment. Ok, agreed. Adding this indirection layer is probably making things a bit too complex. I'll update the patch to clearly separate the std symbol mapping and canonical includes. In D67172#1660435 , @hokein wrote: > And I'm n

[PATCH] D67172: [clangd] Use pre-populated mappings for standard symbols

ilya-biryukov updated this revision to Diff 219061. ilya-biryukov added a comment. - Move system symbol mapping to a different class. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67172/new/ https://reviews.llvm.org/D67172 Files: clang-tools-ext

[PATCH] D67172: [clangd] Use pre-populated mappings for standard symbols

ilya-biryukov added a comment. Not sure whether the new code is a win overall, it's still a bit messy Let me know what you think. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67172/new/ https://reviews.llvm.org/D67172 __

[PATCH] D66516: [clangd] Added highlighting to types dependant on templates.

ilya-biryukov updated this revision to Diff 219090. ilya-biryukov added a comment. - Replace UnderlyingTypeVisitor with a function - Remove PrintTo() - Also highlight references to typedefs - reformat Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66

[PATCH] D66516: [clangd] Added highlighting to types dependant on templates.

ilya-biryukov planned changes to this revision. ilya-biryukov added a comment. Now that I think about it more, maybe the simplest option is to not recurse into composite types at all E.g. a typedef to a template parameter will be highlighted as a template parameter. A typedef to a pointer ca

[PATCH] D67274: [clangd] Improve output of semantic highlighting tests in case of failures

ilya-biryukov created this revision. ilya-biryukov added a reviewer: hokein. Herald added subscribers: kadircet, arphaman, jkorous, MaskRay. Herald added a project: clang. Instead of matching lists of highlightings, we annotate input code with resulting highlightings and diff it against the expect

[PATCH] D66516: [clangd] Added highlighting to types dependant on templates.

ilya-biryukov updated this revision to Diff 219100. ilya-biryukov added a comment. - Only highlight typedefs to non-composite types, simplifies everything Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66516/new/ https://reviews.llvm.org/D66516 Fil

[PATCH] D66516: [clangd] Highlight typedefs to template parameters as template parameters

ilya-biryukov added a comment. I'll send a separate change to fallback to a new highlighting type if the underlying type of a typedef is complicated. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66516/new/ https://reviews.llvm.org/D66516 _

[PATCH] D67277: [clangd] Replace HighlightingKind::NumKinds with LastKind. NFC

ilya-biryukov created this revision. ilya-biryukov added a reviewer: hokein. Herald added subscribers: kadircet, arphaman, jkorous, MaskRay. Herald added a project: clang. The latter simplifies the client code by avoiding the need to handle it as a separate case statement. Repository: rG LLVM

[PATCH] D67290: [clangd] Add a new highlighting kind for typedefs

ilya-biryukov marked an inline comment as done. ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:530 + case HighlightingKind::Typedef: +return "entity.name.type.typedef.cpp"; case HighlightingKind::Namespace: -

[PATCH] D67290: [clangd] Add a new highlighting kind for typedefs

ilya-biryukov created this revision. ilya-biryukov added a reviewer: hokein. Herald added subscribers: kadircet, arphaman, jkorous, MaskRay. Herald added a project: clang. ilya-biryukov added a parent revision: D66516: [clangd] Highlight typedefs to template parameters as template parameters. ilya

[PATCH] D67341: [clangd] Simplify semantic highlighting visitor

ilya-biryukov created this revision. ilya-biryukov added a reviewer: hokein. Herald added subscribers: kadircet, arphaman, jkorous, MaskRay. Herald added a project: clang. - Functions to compute highlighting kinds for things are separated from the ones that add highlighting tokens. This keeps eac

[PATCH] D67277: [clangd] Replace HighlightingKind::NumKinds with LastKind. NFC

ilya-biryukov added a comment. In D67277#1662669 , @hokein wrote: > LGTM, though I think the current implementation is a common pattern. Thanks. It's indeed a common pattern. The problem is that it forces clients to handle this extra case in every swit

[PATCH] D66516: [clangd] Highlight typedefs to template parameters as template parameters

ilya-biryukov updated this revision to Diff 219301. ilya-biryukov marked 5 inline comments as done. ilya-biryukov added a comment. - Address comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66516/new/ https://reviews.llvm.org/D66516 Files:

[PATCH] D66516: [clangd] Highlight typedefs to template parameters as template parameters

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:129 if (const auto *TSI = TD->getTypeSourceInfo()) - addType(TD->getLocation(), TSI->getTypeLoc().getTypePtr()); + addType(TD->getLocation(), TSI->getTypeLoc().getTyp

[PATCH] D67172: [clangd] Use pre-populated mappings for standard symbols

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/index/CanonicalIncludes.cpp:780 + // Prefer a mapping from the system symbols. + if (SystemSymbols) { +if (auto Result = SystemSymbols->mapHeader(Header, QualifiedName)) hokein wrote:

[PATCH] D67172: [clangd] Use pre-populated mappings for standard symbols

ilya-biryukov added a comment. In D67172#1662888 , @sammccall wrote: > I do wonder whether we're microoptimizing for the tests too much, I don't > think 5% on the tests is worth very much in itself, unless it's speeding up > real workloads or improving t

[PATCH] D67264: [clangd] Collect location of macro definition in the ParsedAST

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/ParsedAST.cpp:104 // *definitions* in the preamble region of the main file). class CollectMainFileMacroExpansions : public PPCallbacks { const SourceManager &SM; hokein wrote: > The na

[PATCH] D67172: [clangd] Use pre-populated mappings for standard symbols

ilya-biryukov added a comment. In D67172#1662945 , @sammccall wrote: > Only if it's 5% of something meaningful. If the denominator isn't something > we care about, then it's really "spending XXX usec is not ok" - which depends > on XXX I think. Agree,

[PATCH] D67172: [clangd] Use pre-populated mappings for standard symbols

ilya-biryukov updated this revision to Diff 219335. ilya-biryukov added a comment. - Alternative approach - do not add extra classes, aim to minimize changes Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67172/new/ https://reviews.llvm.org/D67172

[PATCH] D67172: [clangd] Use pre-populated mappings for standard symbols

ilya-biryukov added a comment. The new version looks very much like the older one, with a few additions to not re-initialize data twice. PTAL and let me know what you think. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67172/new/ https://reviews.

[PATCH] D67172: [clangd] Use pre-populated mappings for standard symbols

ilya-biryukov marked an inline comment as done. ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/index/CanonicalIncludes.cpp:748 +llvm::StringRef CanonicalPath = Pair.second; +int Components = std::distance(llvm::sys::path::begin(Suffix), +

[PATCH] D67290: [clangd] Add a new highlighting kind for typedefs

ilya-biryukov updated this revision to Diff 219348. ilya-biryukov marked 3 inline comments as done. ilya-biryukov added a comment. - Remove stale comment Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67290/new/ https://reviews.llvm.org/D67290 File

[PATCH] D67290: [clangd] Add a new highlighting kind for typedefs

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:530 + case HighlightingKind::Typedef: +return "entity.name.type.typedef.cpp"; case HighlightingKind::Namespace: hokein wrote: > ilya-biryukov wrote: > > Not

[PATCH] D67172: [clangd] Use pre-populated mappings for standard symbols

ilya-biryukov added a comment. In D67172#1663096 , @sammccall wrote: > LG, though if we can drop the struct and make MaxSuffixComponents a constant > it'd be simpler still. Done. > Sure. This is going to win a couple of percent I guess: for these cases

[PATCH] D67172: [clangd] Use pre-populated mappings for standard symbols

ilya-biryukov updated this revision to Diff 219360. ilya-biryukov marked 3 inline comments as done. ilya-biryukov added a comment. - Make MaxSuffixComponents a constant - Put the suffix mapping into a single constant - Initialize all StringMaps directly - Rename to Std...Mapping Repository: rG

[PATCH] D67172: [clangd] Use pre-populated mappings for standard symbols

ilya-biryukov marked 8 inline comments as done. ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/index/CanonicalIncludes.cpp:41 + if (!SuffixHeaderMapping) +return Header; sammccall wrote: > nit: can we write `if (SuffixHeaderMappi

[PATCH] D67172: [clangd] Use pre-populated mappings for standard symbols

ilya-biryukov added a comment. Thanks for all suggestions. The final result is rather small and minimal. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67172/new/ https://reviews.llvm.org/D67172 ___ cfe

[PATCH] D67341: [clangd] Simplify semantic highlighting visitor

ilya-biryukov updated this revision to Diff 219363. ilya-biryukov added a comment. - Turn into NFC, do not highlight lambdas differently Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67341/new/ https://reviews.llvm.org/D67341 Files: clang-tools-

[PATCH] D67341: [clangd] Simplify semantic highlighting visitor

ilya-biryukov added a comment. In D67341#1663104 , @hokein wrote: > Unfortunately, the patch is a bit large, containing refactoring changes and > functionality changes, is it possible to split it into (two) smaller patches? Done. There should be no func

[PATCH] D67264: [clangd] Collect location of macro definition in the ParsedAST

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM. We should probably also take a look at highlighting macros inside the preamble part of the main file. @hokein, are you planning to do this or should we just file a bug for

[PATCH] D35986: [clangd] Add ':' to completion trigger characters.

ilya-biryukov created this revision. Without it we don't get completion requests from VSCode after nested name qualifiers (e.g. after 'std::'). https://reviews.llvm.org/D35986 Files: clangd/ClangdLSPServer.cpp Index: clangd/ClangdLSPServer.cpp ===

[PATCH] D36095: [clangd] Allow to get vfs::FileSystem used inside codeComplete.

ilya-biryukov created this revision. This is useful for managing lifetime of VFS-based caches. https://reviews.llvm.org/D36095 Files: clangd/ClangdServer.cpp clangd/ClangdServer.h Index: clangd/ClangdServer.h === --- clangd/C

[PATCH] D36095: [clangd] Allow to get vfs::FileSystem used inside codeComplete.

ilya-biryukov added a comment. In https://reviews.llvm.org/D36095#826275, @bkramer wrote: > lg (it could use a test case though) Will add a test case in one of further commits. Repository: rL LLVM https://reviews.llvm.org/D36095 ___ cfe-commit

[PATCH] D36133: [clangd] Rewrote AST and Preamble management.

ilya-biryukov created this revision. The new implementation allows code completion that never waits for AST. https://reviews.llvm.org/D36133 Files: clangd/ClangdServer.cpp clangd/ClangdServer.h clangd/ClangdUnit.cpp clangd/ClangdUnit.h clangd/ClangdUnitStore.cpp clangd/ClangdUnitSto

[PATCH] D36133: [clangd] Rewrote AST and Preamble management.

ilya-biryukov added a comment. I also want to rename ClangdUnit and ClangdUnitStore accordingly, but will do that in a separate commit so that git-svn correctly detects the renames (i.e. don't want file contents changes). https://reviews.llvm.org/D36133 _

[PATCH] D36133: [clangd] Rewrote AST and Preamble management.

ilya-biryukov updated this revision to Diff 109091. ilya-biryukov added a comment. - Fixed a bug that caused CppFiles to be deleted while used. https://reviews.llvm.org/D36133 Files: clangd/ClangdServer.cpp clangd/ClangdServer.h clangd/ClangdUnit.cpp clangd/ClangdUnit.h clangd/ClangdU

[PATCH] D36133: [clangd] Rewrote AST and Preamble management.

ilya-biryukov updated this revision to Diff 109124. ilya-biryukov added a comment. Addressed review comments. - Moved implementations of template function to header. - Fixed a typo. https://reviews.llvm.org/D36133 Files: clangd/ClangdServer.cpp clangd/ClangdServer.h clangd/ClangdUnit.cpp

[PATCH] D36133: [clangd] Rewrote AST and Preamble management.

ilya-biryukov marked 3 inline comments as done. ilya-biryukov added inline comments. Comment at: clangd/ClangdServer.h:113 + /// queue. The request will be run on a separate thread. + template void addToFront(Func &&F, Args &&... As); + /// Add a new request to run function

[PATCH] D36155: Use VFS operations in FileManager::makeAbsolutePath.

ilya-biryukov created this revision. It used to call into llvm::sys::fs::make_absolute. https://reviews.llvm.org/D36155 Files: lib/Basic/FileManager.cpp unittests/Basic/FileManagerTest.cpp Index: unittests/Basic/FileManagerTest.cpp =

[PATCH] D36150: [clangd] LSP extension to switch between source/header file

ilya-biryukov requested changes to this revision. ilya-biryukov added a reviewer: ilya-biryukov. ilya-biryukov added a comment. Also, +1 to the comments about file extensions, we have to cover at least `.c`, `.cc` and `.cpp` for source files, `.h` and `.hpp` for header files.

[PATCH] D35894: [clangd] Code hover for Clangd

ilya-biryukov added a reviewer: ilya-biryukov. ilya-biryukov requested changes to this revision. ilya-biryukov added a comment. I just wanted to take a step back and discuss what we want to get from code hover in the first place. I would expect a typical code hover to be a bit more involved and

[PATCH] D35894: [clangd] Code hover for Clangd

ilya-biryukov added a comment. > I think all of those would be great. Our objective is to bring basic but > correct features that will put us close to parity with Eclipse CDT, so that > our users can transition. In CDT only the "raw" source is shown, similar to > this patch (except in CDT it at

[PATCH] D36226: Added a RealFileSystem implementation that does not rely on global CWD.

ilya-biryukov created this revision. Allows to have multiple instances of RealFileSystem that have different working directories. https://reviews.llvm.org/D36226 Files: include/clang/Basic/VirtualFileSystem.h lib/Basic/VirtualFileSystem.cpp unittests/Basic/VirtualFileSystemTest.cpp Index

[PATCH] D36226: Added a RealFileSystem implementation that does not rely on global CWD.

ilya-biryukov added a comment. Also tested by temporarily replacing `getRealFileSystem()` body with `return createThreadFriendlyRealFS();` and running `check-all`, all tests passed. To be more specific, there was one crash in clang-format, as there is a piece of code that uses raw pointer from `

[PATCH] D36261: [clangd] Use multiple working threads in clangd.

ilya-biryukov created this revision. https://reviews.llvm.org/D36261 Files: clangd/ClangdLSPServer.cpp clangd/ClangdLSPServer.h clangd/ClangdServer.cpp clangd/ClangdServer.h clangd/tool/ClangdMain.cpp unittests/clangd/ClangdTests.cpp Index: unittests/clangd/ClangdTests.cpp ==

[PATCH] D36226: Added a RealFileSystem implementation that does not rely on global CWD.

ilya-biryukov planned changes to this revision. ilya-biryukov added a comment. After a chat with @klimek, working on an implementation that would use `openAt()`. https://reviews.llvm.org/D36226 ___ cfe-commits mailing list cfe-commits@lists.llvm.or

[PATCH] D35894: [clangd] Code hover for Clangd

ilya-biryukov added a comment. In https://reviews.llvm.org/D35894#829190, @malaperle wrote: > @Nebiroth I think it's OK to put this on hold until we make the "semantic" > hover and figure out how to have both. From our perspective, this is going > beyond parity of what we had before but it's se

[PATCH] D36226: Added a RealFileSystem implementation that does not rely on global CWD.

ilya-biryukov updated this revision to Diff 109524. ilya-biryukov added a comment. Added a very rough prototype of vfs::RealFileSystem not using openat. Not ready for submission yet, wanted to start discussion about Windows implementation. https://reviews.llvm.org/D36226 Files: include/clang

[PATCH] D36226: Added a RealFileSystem implementation that does not rely on global CWD.

ilya-biryukov added a comment. Requires changes in Support library to run correctly: https://reviews.llvm.org/D36265 https://reviews.llvm.org/D36226 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listi

[PATCH] D36261: [clangd] Use multiple working threads in clangd.

ilya-biryukov added inline comments. Comment at: clangd/ClangdServer.h:121-122 +public: + /// Returns the number of threads to use when shouldRunsynchronously() is + /// false. Must not be called if shouldRunsynchronously() is true. + unsigned getThreadsCount(); --

[PATCH] D36261: [clangd] Use multiple working threads in clangd.

ilya-biryukov updated this revision to Diff 109527. ilya-biryukov marked 3 inline comments as done. ilya-biryukov added a comment. Addressed review comments. - Fixed typos. - Renamed SchedulingParams to SchedulingOptions. https://reviews.llvm.org/D36261 Files: clangd/ClangdLSPServer.cpp cl

[PATCH] D36261: [clangd] Use multiple working threads in clangd.

ilya-biryukov updated this revision to Diff 109532. ilya-biryukov added a comment. - Removed SchedulingOptions altogether, replaced with AsyncThreadsCount. https://reviews.llvm.org/D36261 Files: clangd/ClangdLSPServer.cpp clangd/ClangdLSPServer.h clangd/ClangdServer.cpp clangd/ClangdSer

[PATCH] D36261: [clangd] Use multiple working threads in clangd.

ilya-biryukov added inline comments. Comment at: clangd/tool/ClangdMain.cpp:69 + SchedulingOptions SchedOpts = + !RunSynchronously ? SchedulingOptions::RunOnWorkerThreads(ThreadsCount) +: SchedulingOptions::RunOnCallingThread(); kras

[PATCH] D36261: [clangd] Use multiple working threads in clangd.

ilya-biryukov updated this revision to Diff 109534. ilya-biryukov marked 4 inline comments as done. ilya-biryukov added a comment. - Fixed more typos/inconsistences in comments, pointed out by @krasimir. https://reviews.llvm.org/D36261 Files: clangd/ClangdLSPServer.cpp clangd/ClangdLSPServe

[PATCH] D36150: [clangd] LSP extension to switch between source/header file

ilya-biryukov requested changes to this revision. ilya-biryukov added a comment. This revision now requires changes to proceed. This is probably not a final version, but I feel my comments should be helpful anyway. Also, we're missing testcases now. Comment at: clangd/ClangdLS

[PATCH] D36397: [clangd] Fixed a data race.

ilya-biryukov created this revision. Calling addDocument after removeDocument could have resulted in an invalid program state (AST and Preamble for the valid document could have been incorrectly removed). This commit also includes an improved CppFile::cancelRebuild implementation that allows to ca

[PATCH] D36398: [clangd] Check if CompileCommand has changed on forceReparse.

ilya-biryukov created this revision. https://reviews.llvm.org/D36398 Files: clangd/ClangdServer.cpp clangd/ClangdServer.h clangd/ClangdUnitStore.cpp clangd/ClangdUnitStore.h Index: clangd/ClangdUnitStore.h === --- clangd/Cla

[PATCH] D36397: [clangd] Fixed a data race.

ilya-biryukov added a comment. In https://reviews.llvm.org/D36397#833883, @klimek wrote: > Tests? TSAN does not catch this (as it's a logical error) and it requires a rather bizarre timing of file reparses to trigger. I couldn't come up with an example that would reproduce this. https://revi

[PATCH] D36398: [clangd] Check if CompileCommand has changed on forceReparse.

ilya-biryukov updated this revision to Diff 110016. ilya-biryukov added a comment. - Moved assignment `RemoveFile = nullptr` around a bit. - Added a comment to recreateFileIfCompileCommandChanged. https://reviews.llvm.org/D36398 Files: clangd/ClangdServer.cpp clangd/ClangdServer.h clangd/

[PATCH] D36398: [clangd] Check if CompileCommand has changed on forceReparse.

ilya-biryukov added inline comments. Comment at: clangd/ClangdUnitStore.cpp:45 + .first; +Result.RemovedFile = nullptr; + } else if (!compileCommandsAreEqual(It->second->getCompileCommand(), klimek wrote: > Just say RemovedFile = nullptr in the s

[PATCH] D36261: [clangd] Use multiple working threads in clangd.

ilya-biryukov added inline comments. Comment at: clangd/ClangdServer.h:121-122 +public: + /// Returns the number of threads to use when shouldRunsynchronously() is + /// false. Must not be called if shouldRunsynchronously() is true. + unsigned getThreadsCount(); --

[PATCH] D36261: [clangd] Use multiple working threads in clangd.

ilya-biryukov added a comment. In https://reviews.llvm.org/D36261#834902, @klimek wrote: > High-level question: Why can't we use llvm::ThreadPool? It is not an in-place replacement as it does not allow to prioritize new tasks over old ones (new tasks are usually more important for clangd as th

[PATCH] D36398: [clangd] Check if CompileCommand has changed on forceReparse.

ilya-biryukov updated this revision to Diff 110155. ilya-biryukov added a comment. Addressed review comments. - Added a test for forceReparse. - Got rid of a redundant `= nullptr` assignment. https://reviews.llvm.org/D36398 Files: clangd/ClangdServer.cpp clangd/ClangdServer.h clangd/Clan

[PATCH] D36398: [clangd] Check if CompileCommand has changed on forceReparse.

ilya-biryukov marked an inline comment as done. ilya-biryukov added a comment. In https://reviews.llvm.org/D36398#834904, @klimek wrote: > Also missing tests :) Done :-) Comment at: clangd/ClangdUnitStore.h:45-48 + struct RecreateResult { +std::shared_ptr FileInCollecti

[PATCH] D36261: [clangd] Use multiple working threads in clangd.

ilya-biryukov updated this revision to Diff 110381. ilya-biryukov added a comment. - Added a stress test for multithreading. https://reviews.llvm.org/D36261 Files: clangd/ClangdLSPServer.cpp clangd/ClangdLSPServer.h clangd/ClangdServer.cpp clangd/ClangdServer.h clangd/tool/ClangdMain.

[PATCH] D36261: [clangd] Use multiple working threads in clangd.

ilya-biryukov added a comment. Added a test that exercises some multi-threading behaviour. It really finds bugs, at least it found a problem that was fixed by https://reviews.llvm.org/D36397. (Will make sure to submit that other change before this one). As discussed with @klimek, will move to

[PATCH] D36397: [clangd] Fixed a data race.

ilya-biryukov added a comment. In https://reviews.llvm.org/D36397#833912, @klimek wrote: > Yea, we'll probably want to add a "smash it hard with parallel" requests kind > of test to catch these things. You're right, there is probably not a better > solution for this specific bug. Added a rele

[PATCH] D36529: Fixed a race condition in PrecompiledPreamble.

ilya-biryukov added a comment. Catched by a test from https://reviews.llvm.org/D36261. https://reviews.llvm.org/D36529 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D36529: Fixed a race condition in PrecompiledPreamble.

ilya-biryukov created this revision. Two PrecompiledPreambles, used in parallel on separate threads, could be writing preamble to the same temporary file. https://reviews.llvm.org/D36529 Files: lib/Frontend/PrecompiledPreamble.cpp Index: lib/Frontend/PrecompiledPreamble.cpp

[PATCH] D36529: Fixed a race condition in PrecompiledPreamble.

ilya-biryukov added inline comments. Comment at: lib/Frontend/PrecompiledPreamble.cpp:470-471 + int FD; + auto EC = llvm::sys::fs::createTemporaryFile(Prefix, Suffix, /*ref*/ FD, + /*ref*/ File); if (EC) klimek w

[PATCH] D36397: [clangd] Fixed a data race.

ilya-biryukov added inline comments. Comment at: clangd/ClangdUnit.cpp:714 -void CppFile::cancelRebuilds() { +void CppFile::cancelRebuild() { deferCancelRebuild().get(); } + klimek wrote: > Somewhat orthogonal question - why CppFile? We do support other languag

[PATCH] D36397: [clangd] Fixed a data race.

ilya-biryukov added a comment. A follow-up after discussion offline: we should make an attempt to simplify the threading code in `ClangdUnit.h` by moving the async worker loop of `CppFile` into the class itself, rather than relying on external scheduling to do the right thing. @klimek, are ther

[PATCH] D36397: [clangd] Fixed a data race.

ilya-biryukov added a comment. Thanks. I'll make sure to address the awkwardness in further commits. https://reviews.llvm.org/D36397 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D35200: Don't use mmap on Windows

ilya-biryukov added a comment. This looks strange. AFAIK, memory mapping files in Windows does no 'locking' by itself. I've just written a small program to check that it's possible to modify the file, memory-mapped using Win32 API. What is specific problem you ran into? https://reviews.llvm.or

[PATCH] D35200: Don't use mmap on Windows

ilya-biryukov added a comment. In https://reviews.llvm.org/D35200#844132, @yvvan wrote: > The files might also be removed. > I checked that with qtcreator from git. Switching to the different branch > started reparse of some files. > When I frequently switched branches back and forth I got loc

[PATCH] D36150: [clangd] LSP extension to switch between source/header file

ilya-biryukov added inline comments. Comment at: clangd/ClangdServer.cpp:295 + std::string DEFAULT_SOURCE_EXTENSIONS[] = { ".cpp", ".c", ".cc", ".cxx", +".c++", ".C", ".m", ".mm" }; + std::string DEFAULT_HEADER_EXTENSIONS[] = { ".h", ".hh", ".hpp", ".hxx", ---

[PATCH] D36828: Updated a usage of createTemporaryFile that does not expect file to be created.

ilya-biryukov created this revision. This fixes a usage of createTemporaryFile in clang repo after a change in llvm repo. https://reviews.llvm.org/D36828 Files: unittests/Tooling/ToolingTest.cpp Index: unittests/Tooling/ToolingTest.cpp ===

[PATCH] D36828: Updated a usage of createTemporaryFile that does not expect file to be created.

ilya-biryukov added a comment. A change to llvm repository is here: https://reviews.llvm.org/D36827 https://reviews.llvm.org/D36828 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D36458: Fix crash when current lexer is nullptr

ilya-biryukov added a comment. I've also stumbled upon this crash. Here is an even simpler repro that crashes for me (does not involve includes): #ifndef HEADER_GUARD #define FOO(X) int fhjdfhsdjkfhjkshfjk; FOO(base) struct X { int a; }; int test() { X v; v. }

[PATCH] D36458: Fix crash when current lexer is nullptr

ilya-biryukov added inline comments. Comment at: lib/Parse/Parser.cpp:519 ConsumeToken(); - - PP.replayPreambleConditionalStack(); + if (!PP.isCurrentLexer(nullptr)) +PP.replayPreambleConditionalStack(); ilya-biryukov wrote: > This certainly fixes the cr

[PATCH] D36872: Fixed a crash on replaying Preamble's PP conditional stack.

ilya-biryukov created this revision. The crash occurs when the first token after a preamble is a macro expansion. Fixed by moving replayPreambleConditionalStack from Parser into Preprocessor. It is now called right after the predefines file is processed. https://reviews.llvm.org/D36872 Files:

[PATCH] D36458: Fix crash when current lexer is nullptr

ilya-biryukov added a comment. Here's an alternative fix that moves the `replay...` call into `Preprocessor`: https://reviews.llvm.org/D36872. It also fixes the crash and doesn't add invalid compiler errors about non-matching `#ifdefs`. https://reviews.llvm.org/D36458 __

[PATCH] D36872: Fixed a crash on replaying Preamble's PP conditional stack.

ilya-biryukov marked an inline comment as done. ilya-biryukov added a comment. In https://reviews.llvm.org/D36872#845364, @erikjv wrote: > Can you check if the example in https://bugs.llvm.org/show_bug.cgi?id=33574 > works correctly? Works correctly, added an extra test for it. https://revie

[PATCH] D36872: Fixed a crash on replaying Preamble's PP conditional stack.

ilya-biryukov updated this revision to Diff 111676. ilya-biryukov added a comment. - Removed redundant 'private' specifier. - Added a test for https://bugs.llvm.org/show_bug.cgi?id=33574 https://reviews.llvm.org/D36872 Files: include/clang/Lex/Preprocessor.h lib/Lex/PPLexerChange.cpp lib/

[PATCH] D36150: [clangd] LSP extension to switch between source/header file

ilya-biryukov added a comment. The function in `ClangdServer` seems to be getting more complicated and more complicated, mostly because of mixing up `std::string`, `StringRef`, `SmallString`. Here's a slightly revamped implementation of your function, hope you'll find it (or parts of it) useful

[PATCH] D37101: [clangd] [WIP] Add support for snippet completions

ilya-biryukov requested changes to this revision. ilya-biryukov added a comment. This revision now requires changes to proceed. Thanks for taking your time to implement this! We need to add some test cases for new completion features. However, adding them may be a bit of a pain, so feel free to

[PATCH] D37101: [clangd] [WIP] Add support for snippet completions

ilya-biryukov added a comment. In https://reviews.llvm.org/D37101#851380, @rwols wrote: > Thanks for the quick review! I'm new to Phabricator and the `arc` CLI tool. > Is the workflow like this: "address a comment, change a few lines, do `arc > diff`, do this multiple times", or is it like this

[PATCH] D71920: [AST] Refactor propagation of dependency bits. NFC

ilya-biryukov added a comment. Herald added a subscriber: rnkovacs. @rsmith, could you please take a look and let me know whether you think adding a new type for this makes sense? On one hand, I feel it's good to have a type to represent "dependencies" and it allow to write helper functions and

[PATCH] D71920: [AST] Refactor propagation of dependency bits. NFC

ilya-biryukov marked an inline comment as done. ilya-biryukov added inline comments. Comment at: clang-tools-extra/clang-tidy/hicpp/ExceptionBaseclassCheck.cpp:17 +using namespace clang::ast_matchers; + NOTE: We need this to fix compiler errors down below. Ther

[PATCH] D71920: [AST] Refactor propagation of dependency bits. NFC

ilya-biryukov added a comment. Also note that this change does not move any code around to make sure this change is easy to review and validate that the code is doing the same thing. I'm also planning to move all the code that computes dependencies into one place (it's currently scattered around

<    21   22   23   24   25   26   27   28   29   30   >