[PATCH] D50193: Added functionality to suggest FixIts for conversion of '->' to '.' and vice versa.

ilya-biryukov added inline comments. Comment at: clangd/CodeComplete.cpp:286 +Completion.FixIts.push_back( +toTextEdit(FixIt, ASTCtx.getSourceManager(), {})); + } IIRC LangOptions are actually important when running lexer (that is used i

[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name

ilya-biryukov added a comment. This revision got 'reopened' and is now in the list of accepted revision. Should we close it again? Repository: rC Clang https://reviews.llvm.org/D48903 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http:

[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. In https://reviews.llvm.org/D48903#1187605, @simark wrote: > In https://reviews.llvm.org/D48903#1187596, @ilya-biryukov wrote: > > > This revision got 'reopened' and is now in the

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

ilya-biryukov updated this revision to Diff 132967. ilya-biryukov marked 21 inline comments as done. ilya-biryukov added a comment. - Renamed File to AST. - Introduced startTask(). - Moved small methods of ASTWorkerHandle to have inline definitions. - Removed constructor of FileData. - Replaced fi

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

ilya-biryukov added inline comments. Comment at: clangd/ASTWorker.cpp:102 + // not waste time on it. + LastUpdateCF->cancel(); +} sammccall wrote: > ilya-biryukov wrote: > > sammccall wrote: > > > This strategy has some upsides: > > > - we eventual

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

ilya-biryukov updated this revision to Diff 133003. ilya-biryukov added a comment. - Addressed the last review comment Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D42573 Files: clangd/CMakeLists.txt clangd/ClangdServer.h clangd/ClangdUnit.h clangd/ClangdUnitStore.cpp

[PATCH] D42895: [libclang] Add `CXSymbolRole role` to CXIdxEntityRefInfo

ilya-biryukov added inline comments. Comment at: include/clang-c/Index.h:6159 + */ + CXSymbolRole role; } CXIdxEntityRefInfo; Why do we need to store both `CXIdxEntityRefKind` and `CXSymbolRole`? Can we store just `CXSymbolRole`? Is this for compatibility wi

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

ilya-biryukov created this revision. ilya-biryukov added reviewers: sammccall, hokein, ioeric. Herald added subscribers: jkorous-apple, klimek. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D43065 Files: clangd/ClangdServer.h clangd/ClangdUnit.cpp clangd/ClangdUnit.h clan

[PATCH] D42895: [libclang] Add `CXSymbolRole role` to CXIdxEntityRefInfo

ilya-biryukov added inline comments. Comment at: include/clang-c/Index.h:6159 + */ + CXSymbolRole role; } CXIdxEntityRefInfo; MaskRay wrote: > ilya-biryukov wrote: > > Why do we need to store both `CXIdxEntityRefKind` and `CXSymbolRole`? Can > > we store jus

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

ilya-biryukov created this revision. ilya-biryukov added reviewers: hokein, ioeric, sammccall. Herald added subscribers: jkorous-apple, klimek. It was deprecated and callback version and is used everywhere. Only changes to the testing code were needed. Repository: rCTE Clang Tools Extra https

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

ilya-biryukov added a comment. Thanks for picking this up! I have just one major comment related to how we generate the hover text. Current implementation tries to get the actual source code for every declaration and then patch it up to look nice on in the output. It is quite a large piece of co

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

ilya-biryukov added a comment. Thanks for the NITs :-) Comment at: clangd/ClangdUnit.cpp:399 + std::unique_ptr CI; + { +// FIXME(ibiryukov): store diagnostics from CommandLine when we start ioeric wrote: > Do we still need this block? I added it to avoid

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

ilya-biryukov updated this revision to Diff 133416. ilya-biryukov marked 2 inline comments as done. ilya-biryukov added a comment. - Removed braces - s/latest/last/ Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D43065 Files: clangd/ClangdServer.h clangd/ClangdUnit.cpp cla

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

ilya-biryukov added inline comments. Comment at: unittests/clangd/SyncAPI.h:8 +// +//===-===// +#ifndef LLVM_CLANG_TOOLS_EXTRA_UNITTESTS_CLANGD_SYNCAPI_H ioeric wrote: > Any reason not to put syn

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

ilya-biryukov added inline comments. Comment at: unittests/clangd/SyncAPI.h:8 +// +//===-===// +#ifndef LLVM_CLANG_TOOLS_EXTRA_UNITTESTS_CLANGD_SYNCAPI_H ioeric wrote: > ilya-biryukov wrote: > >

[PATCH] D42895: [libclang] Add `CXSymbolRole role` to CXIdxEntityRefInfo

ilya-biryukov added inline comments. Comment at: include/clang-c/Index.h:6159 + */ + CXSymbolRole role; } CXIdxEntityRefInfo; MaskRay wrote: > ilya-biryukov wrote: > > MaskRay wrote: > > > ilya-biryukov wrote: > > > > Why do we need to store both `CXIdxEntity

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

ilya-biryukov created this revision. ilya-biryukov added reviewers: hokein, ioeric, sammccall. Herald added subscribers: jkorous-apple, klimek. This can happen if the CompileCommand provided by compilation database is malformed. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D431

[PATCH] D43123: [clangd] Log all ignored diagnostics.

ilya-biryukov created this revision. ilya-biryukov added reviewers: ioeric, hokein, sammccall. Herald added subscribers: jkorous-apple, klimek. To aid debugging failures and crashes. Only part of ignored diagnostics was logged before, now we log all of them. Repository: rCTE Clang Tools Extra

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

ilya-biryukov updated this revision to Diff 133593. ilya-biryukov marked 4 inline comments as done. ilya-biryukov added a comment. - Removed assert that is now redundant - Moved EXPECT_ERROR to Matchers.h - Added more context into EXPECT_ERROR's error message Repository: rCTE Clang Tools Extra

[PATCH] D42895: [libclang] Add `CXSymbolRole role` to CXIdxEntityRefInfo

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM. > Is it possible to see this landed before clang+llvm 6 is released? I don't know in detail how releases work, but I believe the release branch has already been created an

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

ilya-biryukov updated this revision to Diff 133825. ilya-biryukov marked 2 inline comments as done. ilya-biryukov added a comment. Herald added a subscriber: mgorny. - Added CaptureProxy helper, use it to implement runCodeComplete. - Documented that we deliberately don't expose the sync API in tes

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

ilya-biryukov added inline comments. Comment at: unittests/clangd/SyncAPI.h:1 +//===--- SyncAPI.h - Sync version of ClangdServer's API --*- C++-*-===// +// sammccall wrote: > Being able to call synchronously is really nice for tests. > It's a bit unfortu

[PATCH] D43123: [clangd] Log all ignored diagnostics.

ilya-biryukov added inline comments. Comment at: clangd/ClangdUnit.cpp:197 - -log(llvm::formatv("Ignored diagnostic outside main file. {0}: {1}", - Location, Message)); hokein wrote: > I'm not sure, do we care about this particular case (

[PATCH] D43123: [clangd] Log all ignored diagnostics.

ilya-biryukov updated this revision to Diff 133827. ilya-biryukov marked an inline comment as done. ilya-biryukov added a comment. - Renamed logIgnoredDiag to log. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D43123 Files: clangd/ClangdUnit.cpp clangd/Compiler.cpp clangd

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

ilya-biryukov 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; Maybe remove th

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

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

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

ilya-biryukov added a comment. In https://reviews.llvm.org/D35894#1003356, @simark wrote: > In https://reviews.llvm.org/D35894#1003342, @simark wrote: > > > The only problem I see is that when hovering a function/struct name, it now > > prints the whole function/struct definition. When talking

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

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM Comment at: unittests/clangd/ClangdTests.cpp:783 public: -NoConcurrentAccessDiagConsumer(std::promise StartSecondReparse) -: StartSecondRepa

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

ilya-biryukov created this revision. ilya-biryukov added reviewers: sammccall, ioeric, hokein. Herald added subscribers: jkorous-apple, klimek. As a consequence, all LSP operations are now handled asynchronously, i.e. they never block the main processing thread. However, if -run-synchronously flag

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

ilya-biryukov created this revision. ilya-biryukov added reviewers: hokein, ioeric, sammccall. Herald added subscribers: jkorous-apple, klimek. And remove -enable-snippets flag. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D43229 Files: clangd/ClangdLSPServer.cpp clangd/Pr

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

ilya-biryukov created this revision. ilya-biryukov added reviewers: hokein, ioeric, sammccall. Herald added subscribers: jkorous-apple, klimek. Some of the existing structs had primimtive fields that were not explicitly initialized on construction. After this commit every struct consistently sets

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

ilya-biryukov updated this revision to Diff 134019. ilya-biryukov added a comment. - Removed initializers for Optional<> fields, they are not problematic Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D43230 Files: clangd/Protocol.h Index: clangd/Protocol.h ==

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

ilya-biryukov added a comment. This commit makes potential UB less likely. Logical errors (i.e. forgot to initialize some fields) will still be there. Sending this for review to make sure everyone agrees this is a good thing. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D43230

[PATCH] D38639: [clangd] #include statements support for Open definition

ilya-biryukov added inline comments. Comment at: clangd/ClangdUnit.cpp:103 + +if (SourceMgr.isInMainFile(FilenameRange.getAsRange().getBegin())) { + // Only inclusion directives in the main file make sense. The user cannot NIT: replace `FilenameRange.get

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

ilya-biryukov added a comment. In https://reviews.llvm.org/D39571#1005926, @malaperle wrote: > I haven't looked at the newest patch yet but I gave it a quick try and saw > something odd. If I change the configuration to something invalid (say I > specify the path to a CMakeLists.txt), then I ge

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

ilya-biryukov created this revision. ilya-biryukov added reviewers: hokein, ioeric, sammccall. Herald added subscribers: jkorous-apple, klimek. The assertion will point directly to misbehaving code, so that debugging related problems (like the one fixed by r325029) is easier. Repository: rCTE

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

ilya-biryukov added inline comments. Comment at: clangd/Protocol.h:190 /// (see exit notification) its process. - llvm::Optional processId; + llvm::Optional processId = 0; jkorous-apple wrote: > Sorry if I am asking stupid question but since the type is Op

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

ilya-biryukov added a comment. In https://reviews.llvm.org/D43230#1006104, @ioeric wrote: > But I think it's safe and probably easier to rely on default values of > primitive types like int, bool etc It's not always safe, as primitive types are sometimes left uninitialized (e.g. when construc

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

ilya-biryukov added a comment. In https://reviews.llvm.org/D35894#1006124, @simark wrote: > Is there a way to get the macro name from the MacroInfo object? I couldn't > find any, so the solution I'm considering is to make > `DeclarationAndMacrosFinder::takeMacroInfos` return an > `std::vector

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

ilya-biryukov added inline comments. Comment at: clangd/ClangdLSPServer.cpp:194 +if (!Replacements) + return replyError(ErrorCode::InternalError, +llvm::toString(Replacements.takeError())); ioeric wrote: > nit: since w

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

ilya-biryukov updated this revision to Diff 134080. ilya-biryukov added a comment. - Capture only needed vars in lambda, not everything. - Rebase onto head. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D43227 Files: clangd/ClangdLSPServer.cpp clangd/ClangdServer.cpp clan

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

ilya-biryukov added a comment. In https://reviews.llvm.org/D43227#1006584, @sammccall wrote: > I wonder whether we want to introduce `using Callback = > UniqueFunction` for readability at some point... I agree that's a good idea, will come up with a CL doing that. Comment a

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

ilya-biryukov updated this revision to Diff 134169. ilya-biryukov marked 4 inline comments as done. ilya-biryukov added a comment. - Use llvm::Optional<> in capture() helper to avoid confusion with llvm::Expected. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D43227 Files: c

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

ilya-biryukov updated this revision to Diff 134179. ilya-biryukov marked an inline comment as done. ilya-biryukov added a comment. - Removed initializers for Optional<> fields, they are not problematic - Remove ctor from Position, initialize on per-field basis instead. Repository: rCTE Clang T

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

ilya-biryukov added a comment. In https://reviews.llvm.org/D43230#1006498, @ioeric wrote: > I think it would probably depend on whether we could find a sensible default > value for a field (e.g. is 0 a better default value than UINT_MAX for line > number?) and whether we expect users to always

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

ilya-biryukov updated this revision to Diff 134194. ilya-biryukov added a comment. - Remove URIForFile::setFile(), rely on copy and move constructors instead. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D43246 Files: clangd/ClangdLSPServer.cpp clangd/Protocol.cpp clangd

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

ilya-biryukov added a comment. In https://reviews.llvm.org/D43246#1006525, @ioeric wrote: > I think another option to prevent the bug in r325029 from happening would be > providing a canonical approach for retrieving (absolute) paths from > `FileManager`. We already have code in symbol collecto

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

ilya-biryukov added a comment. In https://reviews.llvm.org/D39571#1006667, @simark wrote: > It seems to me like > > changes in command line arguments (adding -DMACRO=1) are not taken into > account > when changing configuration. It looks like a bug in the preamble handling. (It does not che

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

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LG with a few NITs. Comment at: clangd/Trace.cpp:133 +std::atomic EndTime; // Filled in by endSpan(). +std::string Name; +const uint64_t TID; --

[PATCH] D38639: [clangd] #include statements support for Open definition

ilya-biryukov added inline comments. Comment at: unittests/clangd/XRefsTests.cpp:53 +class IgnoreDiagnostics : public DiagnosticsConsumer { + void onDiagnosticsReady( malaperle wrote: > ilya-biryukov wrote: > > NIT: remove this class, use `IgnoreDiagnostics` f

[PATCH] D38639: [clangd] #include statements support for Open definition

ilya-biryukov added a comment. Last drop of NITs. After they're fixed, this change is ready to land! Comment at: unittests/clangd/XRefsTests.cpp:282 + const char *HeaderContents = R"cpp([[]]int a;)cpp"; + Annotations HeaderAnnoations(HeaderContents); + FS.Files[FooH] = Heade

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

ilya-biryukov added a comment. Just a few last remarks and this is good to go. Should I land it for you after the last comments are fixed? Comment at: clangd/XRefs.cpp:354 + + return Name; +} We should call `flush()` before returning `Name` here. The `raw_stri

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

ilya-biryukov updated this revision to Diff 134405. ilya-biryukov marked an inline comment as done. ilya-biryukov added a comment. - Remove initializers from Optional<> fields Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D43229 Files: clangd/ClangdLSPServer.cpp clangd/Prot

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

ilya-biryukov added inline comments. Comment at: clangd/ClangdLSPServer.cpp:90 void ClangdLSPServer::onInitialize(InitializeParams &Params) { + if (Params.rootUri && !Params.rootUri->file.empty()) +Server.setRootPath(Params.rootUri->file); ioeric wrote: > T

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

ilya-biryukov updated this revision to Diff 134406. ilya-biryukov added a comment. - Rebase onto head Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D43227 Files: clangd/ClangdLSPServer.cpp clangd/ClangdServer.cpp clangd/ClangdServer.h unittests/clangd/ClangdTests.cpp

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

ilya-biryukov added inline comments. Comment at: test/clangd/completion-snippets-disabled.test:1 +# RUN: clangd -lit-test < %s | FileCheck -strict-whitespace %s +# RUN: clangd -lit-test -pch-storage=memory < %s | FileCheck -strict-whitespace %s sammccall wrote:

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

ilya-biryukov updated this revision to Diff 134420. ilya-biryukov added a comment. - Use default values for configuration instead of using Optional<> fields. - Removed redundant tests. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D43229 Files: clangd/ClangdLSPServer.cpp cl

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

ilya-biryukov updated this revision to Diff 134421. ilya-biryukov added a comment. - Removed completion-snippets-missing.test Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D43229 Files: clangd/ClangdLSPServer.cpp clangd/Protocol.cpp clangd/Protocol.h clangd/tool/ClangdM

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

ilya-biryukov added a comment. As discussed offline with @sammccall, we don't really need to hold `Optional<>` fields for configuration entries, as we only consume them in clangd and therefore can just set the default values explicitly. Makes the code simpler and requires only one extra test (wi

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

ilya-biryukov added a comment. Only the naming of fields left. Also, please make sure to run `git-clang-format` on the code before submitting to make sure it's formatted properly. Comment at: clangd/ClangdLSPServer.cpp:331 + if (!H) { +

[PATCH] D38639: [clangd] #include statements support for Open definition

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM modulo the `ASSERT_TRUE` nit. Comment at: unittests/clangd/XRefsTests.cpp:295 + runFindDefinitions(Server, FooCpp, SourceAnnotations.point()); + EXPE

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

ilya-biryukov accepted this revision. ilya-biryukov added a comment. Thanks for fixing all the NITs! Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D35894 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/

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

ilya-biryukov created this revision. ilya-biryukov added reviewers: hokein, ioeric, sammccall. Herald added subscribers: jkorous-apple, klimek. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D43377 Files: clangd/CodeComplete.cpp Index: clangd/CodeComplete.cpp ==

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

ilya-biryukov added a comment. Technically this is NFC, but it has a huge `toString` helper and I'm not sure if I chose the appropriate place for logging the filename. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D43377 ___ cfe-co

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

ilya-biryukov accepted this revision. ilya-biryukov added a comment. LGTM Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D35894 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cf

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

ilya-biryukov updated this revision to Diff 134589. ilya-biryukov marked 2 inline comments as done. ilya-biryukov added a comment. - Attach completion kind in CodeCompleteFlow::run(). - Move printCompletionKind closer to CompletionContext::Kind. - Added FIXME to remove tracing of filename. Repos

[PATCH] D43379: [CodeComplete] Add a helper to print CodeCompletionContext::Kind

ilya-biryukov created this revision. ilya-biryukov added a reviewer: sammccall. Herald added a subscriber: ioeric. Will be used in clangd. See https://reviews.llvm.org/D43377. Repository: rC Clang https://reviews.llvm.org/D43379 Files: include/clang/Sema/CodeCompleteConsumer.h lib/Sema/C

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

ilya-biryukov marked an inline comment as done. ilya-biryukov added inline comments. Comment at: clangd/CodeComplete.cpp:503 +trace::Span Span("Process sema results"); +SPAN_ATTACH(Span, "total_sema_items", NumResults); sammccall wrote: > I think this s

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

ilya-biryukov updated this revision to Diff 134592. ilya-biryukov marked an inline comment as done. ilya-biryukov added a comment. - Rename getFile() to file() - Removed the Path.h include - Rebased onto head Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D43246 Files: clangd/

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

ilya-biryukov marked an inline comment as done. ilya-biryukov added inline comments. Comment at: clangd/Protocol.h:52 struct URIForFile { + URIForFile() = default; sammccall wrote: > ioeric wrote: > > ilya-biryukov wrote: > > > sammccall wrote: > > > > I don'

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

ilya-biryukov added a subscriber: hokein. ilya-biryukov added a comment. LGTM. We'll need to publish new version of VSCode extensions. +@hokein, who did that before. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D43385 ___ cfe-comm

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

ilya-biryukov added a comment. In https://reviews.llvm.org/D39571#1007393, @simark wrote: > If I do it in this order, I get one diagnostic both times, when I don't > expect one the second time the file is parsed. But if I do it the other way > (first parse with no errors, second parse with an

[PATCH] D43454: [clangd] Do not reuse preamble if compile args changed

ilya-biryukov created this revision. ilya-biryukov added reviewers: hokein, ioeric, sammccall. Herald added subscribers: cfe-commits, jkorous-apple, klimek. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D43454 Files: clangd/ClangdUnit.cpp clangd/ClangdUnit.h unittests/clang

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

ilya-biryukov added a comment. In https://reviews.llvm.org/D39571#1011842, @ilya-biryukov wrote: > In https://reviews.llvm.org/D39571#1007393, @simark wrote: > > > If I do it in this order, I get one diagnostic both times, when I don't > > expect one the second time the file is parsed. But if I

[PATCH] D43379: [CodeComplete] Add a helper to print CodeCompletionContext::Kind

ilya-biryukov added inline comments. Comment at: include/clang/Sema/CodeCompleteConsumer.h:355 +/// \brief Get string representation of \p Kind, useful for for debugging. +llvm::StringRef printCompletionKind(enum CodeCompletionContext::Kind Kind); + sammccall wro

[PATCH] D43379: [CodeComplete] Add a helper to print CodeCompletionContext::Kind

ilya-biryukov updated this revision to Diff 134888. ilya-biryukov added a comment. - Rename printCompletionKind to getCompletionKindString Repository: rC Clang https://reviews.llvm.org/D43379 Files: include/clang/Sema/CodeCompleteConsumer.h lib/Sema/CodeCompleteConsumer.cpp Index: lib/

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

ilya-biryukov updated this revision to Diff 134889. ilya-biryukov added a comment. - Rename usage of printCompletionKind to getCompletionKindString - Remove tracing of the filename, TUScheduler now provides those traces Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D43377 Files

[PATCH] D43462: [clangd] Fixes for #include insertion.

ilya-biryukov added inline comments. Comment at: clangd/Headers.cpp:45 +bool hasHeaderExtension(PathRef Path) { + constexpr static const char *HeaderExtensions[] = {".h", ".hpp", ".hh", + ".hxx"}; There is anot

[PATCH] D43462: [clangd] Fixes for #include insertion.

ilya-biryukov added inline comments. Comment at: clangd/Headers.cpp:60 Argv.push_back(S.c_str()); IgnoringDiagConsumer IgnoreDiags; auto CI = clang::createInvocationFromCommandLine( Not related to this patch, but just noticed that we don't call `FS->s

[PATCH] D43462: [clangd] Fixes for #include insertion.

ilya-biryukov added inline comments. Comment at: clangd/index/CanonicalIncludes.cpp:21 llvm::StringRef CanonicalPath) { addRegexMapping((llvm::Twine("^") + llvm::Regex::escape(Path) + "$").str(), CanonicalPath); --

[PATCH] D43462: [clangd] Fixes for #include insertion.

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D43462 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D41537: Optionally add code completion results for arrow instead of dot

ilya-biryukov added a comment. Sorry for not getting back on this earlier. I wanted to discuss whether returning correction as a enum value is a proper interface for users of `libclang`. It seems easy to replace `.` with `->` inside clang, properly handling all the tricky cases (tokens coming f

[PATCH] D43518: [clangd] Allow embedders some control over when diagnostics are generated.

ilya-biryukov added inline comments. Comment at: clangd/ClangdServer.h:282 scheduleReparseAndDiags(PathRef File, VersionedDraft Contents, + WantDiagnostics, Tagged> TaggedFS); Maybe add a parameter name here

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

ilya-biryukov added inline comments. Comment at: unittests/clangd/ClangdTests.cpp:83 +// least one error. +class MultipleErrorCHeckingDiagConsumer : public DiagnosticsConsumer { +public: NIT: misspelling: ErrorCHecking instead of ErrorChecking

[PATCH] D43550: [clangd] Not collect include headers for dynamic index for now.

ilya-biryukov added a comment. Is there a way to still enable include insertion but in a restricted manner, e.g. only for the current project? File of canonical declaration is usually a pretty good guess and it would be nice to have it. Maybe we could allowing to disable the include insertion vi

[PATCH] D43550: [clangd] Not collect include headers for dynamic index for now.

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. As discussed offline, properly supporting IWYU pragma in the dynamic index seems like too much design work for now and it's not gonna get fixed soon. So opting for disabling the f

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

ilya-biryukov accepted this revision. ilya-biryukov added a comment. Thanks for fixing all the comments! LGTM! Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D39571 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists

[PATCH] D41537: Optionally add code completion results for arrow instead of dot

ilya-biryukov added a comment. In https://reviews.llvm.org/D41537#1015563, @yvvan wrote: > Or is your idea is to return the char sequence instead to use this correction > in some universal way? Exactly. Editors that implement corrections would pick up any new corrections automatically, rather

[PATCH] D43518: [clangd] Allow embedders some control over when diagnostics are generated.

ilya-biryukov added inline comments. Comment at: clangd/TUScheduler.cpp:298 + while (shouldSkipHeadLocked()) +Requests.pop_front(); + assert(!Requests.empty() && "skipped the whole queue"); sammccall wrote: > ilya-biryukov wrote: > > Instead of

[PATCH] D43518: [clangd] Allow embedders some control over when diagnostics are generated.

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM (just one more possibly useful nit about const) Comment at: clangd/TUScheduler.cpp:298 + while (shouldSkipHeadLocked()) +Requests.pop_front();

[PATCH] D43569: [clangd] Correct setting ignoreWarnings in CodeCompletion.

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM with a minor nit. Comment at: clangd/CodeComplete.cpp:705 } + CI->getDiagnosticOpts().IgnoreWarnings = true; auto Clang = prepareCompilerInstance( -

[PATCH] D43648: [clangd] Debounce streams of updates.

ilya-biryukov added inline comments. Comment at: clangd/TUScheduler.cpp:140 + // Time to wait after an update to see whether another update obsoletes it. + steady_clock::duration UpdateDebounce; }; Maybe make it `const` and put beside `RunSync`? Both are sched

[PATCH] D43648: [clangd] Debounce streams of updates.

ilya-biryukov added inline comments. Comment at: clangd/TUScheduler.h:56 + std::chrono::steady_clock::duration UpdateDebounce = + std::chrono::milliseconds(500)); ~TUScheduler(); Maybe we could remove this default argument now tha

[PATCH] D33416: [clangd] Allow to use vfs::FileSystem for file accesses.

ilya-biryukov updated this revision to Diff 100065. ilya-biryukov added a comment. Minor refactoring to address @krasimir's comments https://reviews.llvm.org/D33416 Files: clangd/ClangdLSPServer.cpp clangd/ClangdServer.cpp clangd/ClangdServer.h clangd/ClangdUnit.cpp clangd/ClangdUnit.

[PATCH] D33416: [clangd] Allow to use vfs::FileSystem for file accesses.

ilya-biryukov updated this revision to Diff 100262. ilya-biryukov added a comment. - Fixed file comment of ClangdTests.cpp https://reviews.llvm.org/D33416 Files: clangd/ClangdLSPServer.cpp clangd/ClangdServer.cpp clangd/ClangdServer.h clangd/ClangdUnit.cpp clangd/ClangdUnit.h clangd

[PATCH] D33416: [clangd] Allow to use vfs::FileSystem for file accesses.

ilya-biryukov added a comment. Thanks for fixing the failures. Sorry for not being around to look into that myself. Comment at: clang-tools-extra/trunk/unittests/clangd/ClangdTests.cpp:123 + TmpDirs.push_back(TmpDir1.str()); + if (TmpDir2 != TmpDir2) +TmpDirs.push_back(T

[PATCH] D33678: [clangd] Mark results of clangd requests with a tag provided by the FileSystemProvider.

ilya-biryukov created this revision. This allows an implementation of FileSystemProvider that can track which vfs::FileSystem were used for each of the requests. https://reviews.llvm.org/D33678 Files: clangd/ClangdLSPServer.cpp clangd/ClangdServer.cpp clangd/ClangdServer.h unittests/cl

[PATCH] D34033: [clangd] Add parameter and return type information to completion results

ilya-biryukov added inline comments. Comment at: clangd/ClangdUnit.cpp:153 Item.kind = getKind(Result.CursorKind); +Item.insertText = CCS->getTypedText(); if (CCS->getBriefComment()) Should we also update sortText and filterText, which u

[PATCH] D34033: [clangd] Add parameter and return type information to completion results

ilya-biryukov accepted this revision. ilya-biryukov added inline comments. This revision is now accepted and ready to land. Comment at: test/clangd/completion.test:32 # CHECK: {"jsonrpc":"2.0","id":1,"result":[ -# CHECK-DAG: {"label":"a","kind":5} -# CHECK-DAG: {"label":"bb","ki

[PATCH] D34106: [clangd] Use 'std::string' for VFSTag instead of 'int'

ilya-biryukov created this revision. https://reviews.llvm.org/D34106 Files: clangd/ClangdServer.h unittests/clangd/ClangdTests.cpp Index: unittests/clangd/ClangdTests.cpp === --- unittests/clangd/ClangdTests.cpp +++ unittests/c

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