[PATCH] D44293: [clangd] Fix irrelevant declaratations in goto definition (on macros).

ilya-biryukov added inline comments. Comment at: clangd/XRefs.cpp:137 + // + // FIXME: Exclude declarations from macros. + Decls.clear(); NIT: the fixme was a bit hard to follow for me. Maybe make it more clear where the problem should

[PATCH] D44293: [clangd] Fix irrelevant declaratations in goto definition (on macros).

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM with a small nit about the comment. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D44293 ___ cfe-commits mail

[PATCH] D44439: [Sema] Pop function scope when instantiating a func with skipped body

ilya-biryukov created this revision. ilya-biryukov added reviewers: bkramer, sammccall, sepavloff. By calling ActOnFinishFunctionBody(). Previously we were only calling ActOnSkippedFunctionBody, which didn't pop the function scope. This causes a crash when running on our internal code. No test-cas

[PATCH] D44462: [clangd] Don't use DraftMgr in implementation of forceReparse.

ilya-biryukov created this revision. ilya-biryukov added a reviewer: sammccall. Herald added subscribers: ioeric, jkorous-apple, klimek. To make the removal of DraftMgr from ClangdServer easier (https://reviews.llvm.org/D44408). Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D44

[PATCH] D44463: [clangd] Don't expose vfs in TUScheduler::runWithPreamble.

ilya-biryukov created this revision. ilya-biryukov added a reviewer: sammccall. Herald added subscribers: ioeric, jkorous-apple, klimek. It previously an easy way to concurrently access a mutable vfs, which is a recipe for disaster. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/

[PATCH] D44408: Move DraftMgr from ClangdServer to ClangdLSPServer

ilya-biryukov added a reviewer: ilya-biryukov. ilya-biryukov requested changes to this revision. ilya-biryukov added a comment. This revision now requires changes to proceed. We shouldn't add `Contents` parameter to every method, it would defeat the purpose of caching ASTs and won't allow to prop

[PATCH] D44439: [Sema] Pop function scope when instantiating a func with skipped body

ilya-biryukov added a comment. I also came up with a test case, but it breaks because of invalid errors when function bodies are skipped. I'll make sure to include it along with a fix for the errors in a follow-up patch. Comment at: lib/Sema/SemaTemplateInstantiateDecl.cpp:39

[PATCH] D44439: [Sema] Pop function scope when instantiating a func with skipped body

ilya-biryukov updated this revision to Diff 138333. ilya-biryukov marked 4 inline comments as done. ilya-biryukov added a comment. - Addressed review comments Repository: rC Clang https://reviews.llvm.org/D44439 Files: lib/Sema/SemaTemplateInstantiateDecl.cpp Index: lib/Sema/SemaTemplate

[PATCH] D44439: [Sema] Pop function scope when instantiating a func with skipped body

ilya-biryukov added a comment. @aaron.ballman , here's a test-case I came up with. I'll fix the other issue (invalid error, that forces the test to fail) and submit it for review along with the fix for the error. Does that SG? template auto make_func() { struct impl { impl* func(

[PATCH] D44480: [Sema] Don't skip function bodies with 'auto' without trailing return type

ilya-biryukov created this revision. ilya-biryukov added reviewers: aaron.ballman, sammccall. Skipping them was clearly not intentional. It's impossible to guarantee correctness if the bodies are skipped. Also adds a test case for r327504, now that it does not produce invalid errors that made the

[PATCH] D44462: [clangd] Don't use DraftMgr in implementation of forceReparse.

ilya-biryukov updated this revision to Diff 138394. ilya-biryukov marked 2 inline comments as done. ilya-biryukov added a comment. - Remove TUScheduler::updateCompileCommands, add a new optional parameter to clear the cache of compile commands instead - Address review comments Repository: rCT

[PATCH] D44462: [clangd] Don't use DraftMgr in implementation of forceReparse.

ilya-biryukov added inline comments. Comment at: clangd/TUScheduler.h:69 + /// FIXME: remove the callback from this function + void updateCompileCommand(PathRef File, tooling::CompileCommand NewCommand, +IntrusiveRefCntPtr FS, sammcc

[PATCH] D44408: Move DraftMgr from ClangdServer to ClangdLSPServer

ilya-biryukov added a comment. In https://reviews.llvm.org/D44408#1036941, @simark wrote: > I don't see how to avoid adding the Contents parameter to the codeComplete > and signatureHelp methods, since they needed to fetch the document from the > draft manager and pass it to the backend. You

[PATCH] D44462: [clangd] Don't use DraftMgr in implementation of forceReparse.

ilya-biryukov updated this revision to Diff 138397. ilya-biryukov marked 2 inline comments as done. ilya-biryukov added a comment. - Add WantDiagnostics param to runAddDocument - Revert changes in TUScheduler Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D44462 Files: clangd/

[PATCH] D44462: [clangd] Don't use DraftMgr in implementation of forceReparse.

ilya-biryukov added inline comments. Comment at: unittests/clangd/SyncAPI.h:23 +void runAddDocument(ClangdServer &Server, PathRef File, StringRef Contents, +bool SkipCache = false); sammccall wrote: > it's slightly odd that wantdiagnostics i

[PATCH] D44463: [clangd] Don't expose vfs in TUScheduler::runWithPreamble.

ilya-biryukov updated this revision to Diff 138409. ilya-biryukov added a comment. - Rebase on top of master - Add the suggested comment Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D44463 Files: clangd/ClangdServer.cpp clangd/TUScheduler.cpp clangd/TUScheduler.h unitt

[PATCH] D44408: Move DraftMgr from ClangdServer to ClangdLSPServer

ilya-biryukov added a comment. In https://reviews.llvm.org/D44408#1037520, @simark wrote: > I see, I made this as a separate patch: > > https://reviews.llvm.org/D44484 I LGTMed it, so feel free to submit it. However, if you do it in this patch it's also fine. Repository: rCTE Clang Tools Ex

[PATCH] D44408: Move DraftMgr from ClangdServer to ClangdLSPServer

ilya-biryukov added inline comments. Comment at: clangd/ClangdLSPServer.cpp:495 +llvm::Optional ClangdLSPServer::getDocument(PathRef File) { + llvm::Optional Contents = DraftMgr.getDraft(File); + if (!Contents) This function is equivalent to `return DraftMgr.ge

[PATCH] D44408: Move DraftMgr from ClangdServer to ClangdLSPServer

ilya-biryukov added inline comments. Comment at: clangd/ClangdLSPServer.h:78 + /// Calls forceReparse() on all currently opened files. + /// As a result, this method may be very expensive. NIT: there is no `forceReparse()` anymore, maybe remove its mention fr

[PATCH] D44408: Move DraftMgr from ClangdServer to ClangdLSPServer

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/D44408 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D44272: [clangd] Support incremental document syncing

ilya-biryukov added inline comments. Comment at: clangd/ClangdLSPServer.cpp:149 void ClangdLSPServer::onDocumentDidChange(DidChangeTextDocumentParams &Params) { if (Params.contentChanges.size() != 1) return replyError(ErrorCode::InvalidParams, simark wr

[PATCH] D44272: [clangd] Support incremental document syncing

ilya-biryukov added inline comments. Comment at: clangd/ClangdLSPServer.cpp:101 json::obj{ -{"textDocumentSync", 1}, +{"textDocumentSync", 2}, {"documentFormattingProvider", true}, simark wrote: > ilya-biryukov wrote:

[PATCH] D44226: [clangd] Add -log-lsp-to-stderr option

ilya-biryukov added a comment. Sorry for the delay, just a few more comments. Comment at: clangd/ClangdLSPServer.cpp:412 +llvm::raw_string_ostream OS(Message); +OS << "method not found (" << Method << ")"; +replyError(ErrorCode::MethodNotFound, OS.str()); --

[PATCH] D44567: [clangd] Handle multiple callbacks from Sema's completion

ilya-biryukov created this revision. ilya-biryukov added a reviewer: sammccall. Herald added subscribers: ioeric, jkorous-apple, klimek. When parser backtracks, we might receive multiple code completion callbacks. Previously we had a failing assertion there, now we take first results and hope they

[PATCH] D44567: [clangd] Handle multiple callbacks from Sema's completion

ilya-biryukov added a comment. As a follow-up to an offline discussion: I opted for not excluding results from 'Recovery' contexts, because they are actually somewhat semantically relevant (i.e. contain only local variables from the current function). One example where this is useful are skippe

[PATCH] D44567: [clangd] Handle multiple callbacks from Sema's completion

ilya-biryukov updated this revision to Diff 138706. ilya-biryukov marked 2 inline comments as done. ilya-biryukov added a comment. - Address review comments Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D44567 Files: clangd/CodeComplete.cpp unittests/clangd/CodeCompleteTest

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

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. Self-LGTMing since it's a trivial follow-up to an accepted change. https://reviews.llvm.org/D36828 ___ cfe-commits mailing list cfe

[PATCH] D44628: Backport changes from llvm/.clang_tidy to clang/.clang_tidy configs

ilya-biryukov added a subscriber: simark. ilya-biryukov added a comment. Found by @simark while working on https://reviews.llvm.org/D44272. Repository: rC Clang https://reviews.llvm.org/D44628 ___ cfe-commits mailing list cfe-commits@lists.llvm.o

[PATCH] D44628: Backport changes from llvm/.clang_tidy to clang/.clang_tidy configs

ilya-biryukov created this revision. ilya-biryukov added a reviewer: alexfh. ilya-biryukov added a subscriber: simark. ilya-biryukov added a comment. Found by @simark while working on https://reviews.llvm.org/D44272. LLVM .clang_tidy seems to be more up-to-date. Repository: rC Clang https:/

[PATCH] D44272: [clangd] Support incremental document syncing

ilya-biryukov added inline comments. Comment at: clangd/ClangdLSPServer.cpp:164 + if (!Contents) { +log(llvm::toString(Contents.takeError())); +return; We should signal an error to the client by calling `replyError` Comment at: clangd/

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

ilya-biryukov added a comment. Sorry for the delay. Comment at: include/clang/Sema/CodeCompleteConsumer.h:565 + /// \brief For this completion result correction is required. + Optional Corr = None; + Having a string replacement without an actual range to repl

[PATCH] D44628: Backport changes from llvm/.clang_tidy to clang/.clang_tidy configs

ilya-biryukov added inline comments. Comment at: .clang-tidy:8 - key: readability-identifier-naming.FunctionCase -value: lowerCase +value: camelBack + - key: readability-identifier-naming.MemberCase simark w

[PATCH] D44272: [clangd] Support incremental document syncing

ilya-biryukov added inline comments. Comment at: clangd/ClangdLSPServer.cpp:164 + if (!Contents) { +log(llvm::toString(Contents.takeError())); +return; simark wrote: > ilya-biryukov wrote: > > We should signal an error to the client by calling `replyErro

[PATCH] D44673: Make positionToOffset return llvm::Expected

ilya-biryukov added inline comments. Comment at: clangd/ClangdServer.cpp:196 +return Begin.takeError(); + + llvm::Expected End = positionToOffset(Code, Rng.end); NIT: maybe remove empty lines? they don't seem to add much to readability, since when prev line

[PATCH] D47063: [clangd] Keep only a limited number of idle ASTs in memory

ilya-biryukov added inline comments. Comment at: clangd/ClangdUnit.h:143 +std::shared_ptr +buildPreamble(PathRef FileName, CompilerInvocation &CI, + std::shared_ptr OldPreamble, sammccall wrote: > nit: i think filename here is only used for logging,

[PATCH] D47063: [clangd] Keep only a limited number of idle ASTs in memory

ilya-biryukov updated this revision to Diff 149075. ilya-biryukov added a comment. - Fixed formatting Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D47063 Files: clangd/ClangdServer.cpp clangd/ClangdServer.h clangd/ClangdUnit.cpp clangd/ClangdUnit.h clangd/TUScheduler

[PATCH] D44480: [Sema] Don't skip function bodies with 'auto' without trailing return type

ilya-biryukov updated this revision to Diff 149099. ilya-biryukov added a comment. - Fix the comments Repository: rC Clang https://reviews.llvm.org/D44480 Files: lib/Sema/SemaDecl.cpp test/CodeCompletion/crash-skipped-bodies-template-inst.cpp test/CodeCompletion/skip-auto-funcs.cpp In

[PATCH] D47460: Treat files as volatile by default

ilya-biryukov requested changes to this revision. ilya-biryukov added a comment. This revision now requires changes to proceed. It's a very subtle change and it isn't clear if that's the right thing to do without understanding the problem that we're trying to solve. Could you please elaborate on

[PATCH] D44480: [Sema] Don't skip function bodies with 'auto' without trailing return type

ilya-biryukov added a comment. In https://reviews.llvm.org/D44480#1116359, @nik wrote: > I've stumbled about this bug too and was looking into it and then I saw the > mail about this change being submitted :) > > I've ended up with a slightly different change (https://dpaste.de/PSpM/raw , > ins

[PATCH] D47460: [Frontend] Honor UserFilesAreVolatile flag getting file buffer in ASTUnit

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM. Is it plausible to add a unit-test for this? https://reviews.llvm.org/D47460 ___ cfe-commits mailing list cfe-commits@lists.l

[PATCH] D44480: [Sema] Don't skip function bodies with 'auto' without trailing return type

ilya-biryukov added a comment. In https://reviews.llvm.org/D44480#1117230, @nik wrote: > In https://reviews.llvm.org/D44480#1117147, @cpplearner wrote: > > > Does `getAs()` work correctly with function returning `auto&`? > > > the "getAs()" version will skip the function body and generate an > e

[PATCH] D47063: [clangd] Keep only a limited number of idle ASTs in memory

ilya-biryukov added inline comments. Comment at: clangd/TUScheduler.h:66 + std::chrono::steady_clock::duration UpdateDebounce, + ASTRetentionPolicy RetentionPolicy = {}); ~TUScheduler(); sammccall wrote: > ilya-biryukov wrote: > > sam

[PATCH] D47460: [Frontend] Honor UserFilesAreVolatile flag getting file buffer in ASTUnit

ilya-biryukov added a comment. In https://reviews.llvm.org/D47460#1118782, @yvvan wrote: > i think I can add a unit-test for it since we have the 'getBufferKind' method > in MemoryBuffer. That sounds good. Having a regression test that fails with descriptive messages in case anyone changes th

[PATCH] D47623: [clangd] Avoid indexing decls associated with friend decls.

ilya-biryukov added inline comments. Comment at: clangd/index/SymbolCollector.cpp:293 assert(CompletionAllocator && CompletionTUInfo); + // A declaration created for a friend declaration should not be used as the + // canonical declaration in the index. Mayb

[PATCH] D47623: [clangd] Avoid indexing decls associated with friend decls.

ilya-biryukov added a comment. In https://reviews.llvm.org/D47623#1118810, @sammccall wrote: > - friend decls that are not definitions should be ignored for indexing > purposes This is not generally true IIUC. A friend declaration can be a reference, a declaration or a definition. int foo(

[PATCH] D47623: [clangd] Avoid indexing decls associated with friend decls.

ilya-biryukov added a comment. In https://reviews.llvm.org/D47623#1119426, @ioeric wrote: > Sorry for the late response Ilya. I was trying to test these cases. So, with > the current change, if a real "canonical" declaration comes before the friend > decl, then the reference will still be recor

[PATCH] D47707: [clangd] Downrank symbols with reserved names (score *= 0.1)

ilya-biryukov added a comment. Is there a way to only downrank them if the query does not start with `_`? That would cover the cases when I **do** want the symbols starting with underscore. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D47707

[PATCH] D47707: [clangd] Downrank symbols with reserved names (score *= 0.1)

ilya-biryukov added a comment. After chatting offline: a FIXME mentioning some special cases seems enough here, we do improve the results in most cases (i.e. for queries that don't start with an underscore). Another NIT: Maybe we could add unittests for names that start with underscore, but are

[PATCH] D47643: Rewrite JSON dispatcher loop using C IO (FILE*) instead of std::istream.

ilya-biryukov added a subscriber: hokein. ilya-biryukov added a comment. The change LG, but I'm not a big expert on C APIs, so I might've missed something. @ioeric, @hokein, maybe you have some experience with those and want to take a look? PS I've checked it on my Mac and lldb seems to attach

[PATCH] D46943: [clangd] Boost scores for decls from current file in completion

ilya-biryukov updated this revision to Diff 149765. ilya-biryukov marked 5 inline comments as done. ilya-biryukov added a comment. - Address review comments Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D46943 Files: clangd/Quality.cpp clangd/Quality.h unittests/clangd/Qu

[PATCH] D46943: [clangd] Boost scores for decls from current file in completion

ilya-biryukov added inline comments. Comment at: clangd/Quality.h:52 unsigned References = 0; + float ProximityScore = 0.0; /// Proximity score, in a [0,1] interval. sammccall wrote: > this belongs in `SymbolRelevanceSignals` rather than this struct. In >

[PATCH] D47643: Rewrite JSON dispatcher loop using C IO (FILE*) instead of std::istream.

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM Comment at: clangd/JSONRPCDispatcher.cpp:70 } - log(llvm::Twine("--> ") + S); + log(llvm::Twine("--> ") + S + "\n"); } sammccall wro

[PATCH] D46943: [clangd] Boost scores for decls from current file in completion

ilya-biryukov added inline comments. Comment at: unittests/clangd/QualityTests.cpp:129 + Test.Code = R"cpp( +#include "foo.h" +int ::test_func_in_header_and_cpp() { sammccall wrote: > ilya-biryukov wrote: > > sammccall wrote: > > > this is not needed, th

[PATCH] D47871: [clangd] Code completion: drop explicit injected names/operators, ignore Sema priority

ilya-biryukov added a comment. Maybe we could add the test cases that the blacklisted members still show up in completion that don't involve member qualifiers? For example, struct X : std::vector { int test( ){ // <-- 'vector' might be a useful completion here. } }; Reposit

[PATCH] D47896: [CodeComplete] suppress define X X macros

ilya-biryukov added a comment. In https://reviews.llvm.org/D47896#1126171, @sammccall wrote: > Hmm, musl does `#define stderr (stderr)` :-( And they discussed #define > stderr (stderr+0). > (To enforce it's not assigned to etc) > https://github.com/cloudius-systems/musl/blob/master/include/std

[PATCH] D47707: [clangd] Downrank symbols with reserved names (score *= 0.1)

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM, sorry for the delay Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D47707 ___ cfe-commits mailing list cfe-co

[PATCH] D48068: [clangd] Move caching of compile args out of ClangdServer.

ilya-biryukov created this revision. ilya-biryukov added a reviewer: sammccall. Herald added subscribers: jkorous, MaskRay, ioeric, mgorny. Caching is now handled by ClangdLSPServer and hidden behind the GlobalCompilationDatabase interface. This simplifies ClangdServer. No behavioral changes are

[PATCH] D48068: [clangd] Move caching of compile args out of ClangdServer.

ilya-biryukov added a comment. After looking at it more closely, it seems caching happens in the underlying compilation databases anyway. So I guess we don't even need the `CachingCompilationDatabase`. I'll remove it. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D48068 ___

[PATCH] D48068: [clangd] Move caching of compile args out of ClangdServer.

ilya-biryukov updated this revision to Diff 150917. ilya-biryukov added a comment. - Remove CachingCompilationDb Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D48068 Files: clangd/CMakeLists.txt clangd/ClangdLSPServer.cpp clangd/ClangdLSPServer.h clangd/ClangdServer.cpp

[PATCH] D48068: [clangd] Move caching of compile args out of ClangdServer.

ilya-biryukov updated this revision to Diff 150920. ilya-biryukov added a comment. - Revert "Remove CachingCompilationDb". Turns out we do need it internally (Thanks, Sam!) :-) Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D48068 Files: clangd/CMakeLists.txt clangd/ClangdL

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

ilya-biryukov created this revision. ilya-biryukov added a reviewer: sammccall. Herald added subscribers: jkorous, MaskRay, ioeric. Disabled by default and hidden, caching for most implementation already happens outside clangd, so we rarely need to change it. Repository: rCTE Clang Tools Extra

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

ilya-biryukov added a comment. Testing should be possible with lit tests, I'll look into that. Let me know if there's anything else about this patch that needs attention. Thanks! Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D48071 ___

[PATCH] D47950: [clangd] FuzzyMatch: forbid tail-tail matches after a miss: [pat] !~ "panther"

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM with a few nits. And comment requests for things that were unclear when reading the code. Comment at: clangd/FuzzyMatch.cpp:269 + : Awfu

[PATCH] D48083: [clangd] Boost keyword completions.

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/D48083 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

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

ilya-biryukov added inline comments. Comment at: clangd/ClangdLSPServer.h:38 llvm::Optional CompileCommandsDir, - const ClangdServer::Options &Opts); + const ClangdServer::Options &Opts, bool CacheCompileCommands); -

[PATCH] D48068: [clangd] Move caching of compile args out of ClangdServer.

ilya-biryukov updated this revision to Diff 151113. ilya-biryukov marked 2 inline comments as done. ilya-biryukov added a comment. - Address review comments Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D48068 Files: clangd/CMakeLists.txt clangd/ClangdLSPServer.cpp clangd

[PATCH] D48068: [clangd] Move caching of compile args out of ClangdServer.

ilya-biryukov added inline comments. Comment at: clangd/GlobalCompilationDatabase.cpp:137 + // 'It' may be invalid at this point, recompute it. + It = Cached.find(File); + if (It != Cached.end()) sammccall wrote: > `return try_emplace(File, std::move(Command))

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

ilya-biryukov added inline comments. Comment at: clangd/tool/ClangdMain.cpp:141 + "come from the compilation databases."), +llvm::cl::init(false), llvm::cl::Hidden); + sammccall wrote: > ilya-biryukov wrote: > > sammccall wrote: > > > init(t

[PATCH] D48171: [clangd] Do not report comments that only have special chars.

ilya-biryukov created this revision. ilya-biryukov added reviewers: sammccall, ioeric, hokein. Herald added subscribers: jkorous, MaskRay. Like the following: // --- // === // *** It does not cover all the cases, but those are definitely not very useful. Repository: rCTE Cl

[PATCH] D47957: [clangd] Add option to fold overloads into a single completion item.

ilya-biryukov added a comment. Oh, sorry, I forgot to submit the comments yesterday :-( Comment at: clangd/CodeComplete.cpp:245 +// Methods are simply grouped by name. +return hash_combine('M', IndexResult->Name); + case index::SymbolKind::Function: ---

[PATCH] D47957: [clangd] Add option to fold overloads into a single completion item.

ilya-biryukov added a comment. Overall LG. It would be nice to mention somewhere that we since we bundle overloads client-side we might run into weird side-effects (e.g. return less completion items than the specified limit) in case the index returns too many non-bundled. Co

[PATCH] D48211: [clangd] Do not show namespace comments.

ilya-biryukov created this revision. ilya-biryukov added a reviewer: sammccall. Herald added subscribers: jkorous, MaskRay, ioeric. Comments from namespaces that clangd produces are too noisy and often not useful. Namespaces have too many redecls and we don't have a good way of determining which

[PATCH] D47957: [clangd] Add option to fold overloads into a single completion item.

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM. (and a question on why we want to special-case the members) Comment at: clangd/CodeComplete.cpp:245 +// Methods are simply grouped by name. +

[PATCH] D51725: Allow un-setting the compilation database path

ilya-biryukov added a subscriber: sammccall. ilya-biryukov added a comment. In https://reviews.llvm.org/D51725#1255695, @simark wrote: > But I am wondering what to do with the feature that allows clangd to change > compilation database path using the `didChangeConfiguration` notification. > In

[PATCH] D52311: [clangd] Add support for hierarchical documentSymbol

ilya-biryukov added a comment. In https://reviews.llvm.org/D52311#1255876, @simark wrote: > I just tried this, this looks very promising! It should help build our > outline view in a much more robust way than we do currently. > A nit for the final patch, I would suggest omitting the fields tha

[PATCH] D53135: Remove top-level using declaration from header files, as these aliases leak.

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM Comment at: include/clang/Driver/Job.h:33 // Re-export this as clang::driver::ArgStringList. -using llvm::opt::ArgStringList; +using ArgStringList = llvm:

[PATCH] D53347: [clangd] Simplify auto hover

ilya-biryukov created this revision. ilya-biryukov added reviewers: kadircet, hokein. Herald added subscribers: arphaman, jkorous, MaskRay, ioeric. Use helper from clang. Also fixes some weird corner cases, e.g. auto (*foo)() = bar; Repository: rCTE Clang Tools Extra https://reviews.llvm.o

[PATCH] D53192: [clangd] Do not query index for new name completions.

ilya-biryukov added a comment. A drive-by NIT ;-) Comment at: clangd/CodeComplete.cpp:643 case CodeCompletionContext::CCC_Recovery: + // TODO: Provide identifier based completions for the following two contexts: + case CodeCompletionContext::CCC_Name: samm

[PATCH] D52301: [clang] Set TypeSourceInfo for vardecl's in addition to type when we can deduce.

ilya-biryukov added a comment. A few more comments, mostly marking places of unintentional changes that we need to revert. Hope it's not going past the point where the number of comments are not useful. Comment at: include/clang/Basic/DiagnosticSemaKinds.td:8616 "use of %se

[PATCH] D53369: [CodeComplete] Fix accessibility of protected members when accessing members implicitly.

ilya-biryukov accepted this revision. ilya-biryukov added a comment. LGTM, thanks! Would be super-nice if didn't have to rewrite this in code completion Repository: rC Clang https://reviews.llvm.org/D53369 ___ cfe-commits mailing list cfe-commits

[PATCH] D53406: [clangd] Provide excuses for bad code completions, based on diagnostics. C++ API only.

ilya-biryukov added a comment. Since we're showing the diagnostics in the editors anyway, how crucial do we think it is to actually add that to the procotol? Having more concrete reasons for misbehaving completions sounds more useful, though, e.g. "cannot complete members of the incomplete type"

[PATCH] D53456: [Sema] Do not show unused parameter warnings when body is skipped

ilya-biryukov created this revision. ilya-biryukov added reviewers: ioeric, sammccall. Herald added a subscriber: arphaman. Without the function body, we cannot determine is parameter was used. Repository: rC Clang https://reviews.llvm.org/D53456 Files: lib/Sema/SemaDecl.cpp test/Index/s

[PATCH] D53347: [clangd] Simplify auto hover

ilya-biryukov updated this revision to Diff 170273. ilya-biryukov marked 2 inline comments as done. ilya-biryukov added a comment. - Addressed review comments Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53347 Files: clangd/XRefs.cpp unittests/clangd/XRefsTests.cpp Inde

[PATCH] D53347: [clangd] Simplify auto hover

ilya-biryukov added a comment. In https://reviews.llvm.org/D53347#1267216, @kadircet wrote: > LGTM, it bugs me that some part in the documentation says it doesn't go > through decltype(`This looks through declarators like pointer types, but not > through decltype or typedefs`) but since tests c

[PATCH] D53406: [clangd] Provide excuses for bad code completions, based on diagnostics. C++ API only.

ilya-biryukov added inline comments. Comment at: clangd/CodeComplete.cpp:700 + unsigned DiagLoc = Loc.second; + if (DiagLoc < StartOfLine || DiagLoc > Offset) + return; kadircet wrote: > There are also a lot of cases where we can't find an include f

[PATCH] D53635: [CodeComplete] Expose InBaseClass signal in code completion results.

ilya-biryukov added a comment. LG, but could we add a test for the new flag it by printing it in `PrintingCodeCompleteConsumer::ProcessCodeCompleteResults()` and adding corresponding tests to `clang/test/CodeCompletion`? Repository: rC Clang https://reviews.llvm.org/D53635 __

[PATCH] D53635: [CodeComplete] Expose InBaseClass signal in code completion results.

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM with a NIT Comment at: lib/Sema/CodeCompleteConsumer.cpp:557 + if (!Tags.empty()) +OS << " (" << llvm::join(Tags, ",") << ")"; + if (Code

[PATCH] D53638: [clangd] Downrank members from base class

ilya-biryukov added inline comments. Comment at: unittests/clangd/QualityTests.cpp:187 Relevance.merge(CodeCompletionResult(&findDecl(AST, "S::S"), 42)); - EXPECT_EQ(Relevance.Scope, SymbolRelevanceSignals::GlobalScope); } The test case was (accidentally?) r

[PATCH] D53635: [CodeComplete] Expose InBaseClass signal in code completion results.

ilya-biryukov added a comment. And another NIT :-) Comment at: lib/Sema/CodeCompleteConsumer.cpp:548 OS << "COMPLETION: "; +std::vector Tags; switch (Results[I].Kind) { NIT: maybe move Tags into the corresponding case handler? Would require putti

[PATCH] D53641: [clangd] Remove didOpen extraFlags extension.

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM! Thanks for cleaning this up. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53641 ___ cfe-commits mailing li

[PATCH] D53635: [CodeComplete] Expose InBaseClass signal in code completion results.

ilya-biryukov added inline comments. Comment at: lib/Sema/CodeCompleteConsumer.cpp:557 + if (!Tags.empty()) +OS << " (" << llvm::join(Tags, ",") << ")"; + if (CodeCompletionString *CCS = Results[I].CreateCodeCompletionString( ioeric wrote: > ily

[PATCH] D53642: [clangd] Don't invalidate LSP-set compile commands when closing a file.

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

[PATCH] D53644: [clangd] workspace/symbol should be async, it reads from the index.

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/D53644 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D53644: [clangd] workspace/symbol should be async, it reads from the index.

ilya-biryukov added inline comments. Comment at: unittests/clangd/TUSchedulerTests.cpp:541 + std::atomic Counter(0); + S.run("add 1", [&] { Counter.fetch_add(1); }); + S.run("add 2", [&] { Counter.fetch_add(2); }); NIT: maybe simplify to `++Counter`? Reposit

[PATCH] D53654: [clang] Improve ctor initializer completions.

ilya-biryukov added a comment. Great idea, this will definitely improve the UX of completion! NIT: a typo in the change description: 'bas' should be 'base'. Pedantic NIT (sorry!): in the change description, we should probably use 'initializers' instead of 'initializations' Co

[PATCH] D53651: [clangd] Use thread pool for background indexing.

ilya-biryukov added a comment. It's fine to spend one thread spinning on background tasks, but if we're going to do a threadpool, we should be more careful to not hurt the performance of foreground tasks. To do that, we should at least: - share the semaphore for the number of actively running t

[PATCH] D52311: [clangd] Add support for hierarchical documentSymbol

ilya-biryukov updated this revision to Diff 171067. ilya-biryukov added a comment. - Improve traversal of the AST. - Update the tests. - Add a fallback to SymbolInformation (flat structure) if the client does not support the hierarhical reponse. Still some work left to do: - Do not drop the qua

[PATCH] D53687: [clangd] Make in-memory CDB always available as an overlay, refactor.

ilya-biryukov added a comment. FWIW, the old implementation of the CDB looked simpler (which makes sense, since it only allowed the in-memory compile commands, but the new implementation also falls back to the base CDB, i.e. it's now doing two things). However, if we can't avoid this protocol ex

[PATCH] D53688: [clangd] Add fallbackFlags initialization extension.

ilya-biryukov added a comment. This feels like a configuration option that might be changed in the course of running clangd. Are there any strong reasons to make it work only upon initialization? My guess is that it keeps the code simpler, but if we approached it purely from the UX perspective,

[PATCH] D52311: [clangd] Add support for hierarchical documentSymbol

ilya-biryukov added a comment. Another annoyance to fix: - 'using namespace' are shown as '' Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D52311 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin

<    3   4   5   6   7   8   9   10   11   12   >