[PATCH] D84136: [clangd] Fix visitation of ConceptSpecializationExpr in constrained-parameter

kadircet added a comment. just some drive-by comments. Comment at: clang-tools-extra/clangd/unittests/FindTargetTests.cpp:433 + )cpp"; + Flags.push_back("-std=c++20"); + EXPECT_DECLS("ConceptSpecializationExpr", `Flags` is preserved between `EXPECT_DECLS` ca

[PATCH] D84012: [clangd] Exclude preprocessed-to-nothing tokens from selection

kadircet added inline comments. Comment at: clang-tools-extra/clangd/unittests/SelectionTests.cpp:568 +#if 0 +void fu^nc(); +#endif sammccall wrote: > kadircet wrote: > > nit: i am not sure if this is worth it's own test > Moved into CommonAncestor te

[PATCH] D84144: [clangd] Remove TokenBuffer usage in TypeHierarchy

This revision was automatically updated to reflect the committed changes. Closed by commit rGc911803d5df0: [clangd] Remove TokenBuffer usage in TypeHierarchy (authored by ArcsinX, committed by kadircet). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D

[PATCH] D83759: [clangd] Port lit tests to Windows

kadircet added a comment. hi, sorry for the late replay :/ We discussed this offline with Sam and are both concerned with the duplications it introduces, haven't really thought about a nice way to address this yet. As you mentioned the background-index test in itself looks a lot better do, so

[PATCH] D83759: [clangd] Port lit tests to Windows

kadircet added inline comments. Comment at: clang-tools-extra/clangd/test/background-index.test:18 # Test that the index is writing files in the expected location. # RUN: ls %t/.cache/clangd/index/foo.cpp.*.idx # RUN: ls %t/sub_dir/.cache/clangd/index/foo.h.*.idx -

[PATCH] D83759: [clangd] Fixes in lit tests

kadircet accepted this revision. kadircet added a comment. This revision is now accepted and ready to land. thanks, LGTM! as i mentioned please watch out for buildbot breakages after landing this. (and again let me know if I should land this for you) Repository: rG LLVM Github Monorepo CHAN

[PATCH] D83759: [clangd] Fixes in lit tests

kadircet added a comment. In D83759#2165974 , @ArcsinX wrote: > Seem the problem is in `-i` option. > According with OSX man: > `sed [-Ealn] command [file ... ]` > `sed [-Ealn] [-e command] [-f command_file] [-i extension] [file ...]`. > > Seems on macO

[PATCH] D84297: [clangd] Fix Origin and MainFileOnly-ness for macros

kadircet created this revision. kadircet added a reviewer: hokein. Herald added subscribers: cfe-commits, usaxena95, arphaman, jkorous, MaskRay, ilya-biryukov. Herald added a project: clang. This was resulting in macros coming from preambles vanishing when user have opened the source header. For

[PATCH] D84297: [clangd] Fix Origin and MainFileOnly-ness for macros

kadircet updated this revision to Diff 279731. kadircet marked 2 inline comments as done. kadircet added a comment. - Move declarations closer to use. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D84297/new/ https://reviews.llvm.org/D84297 Files:

[PATCH] D84297: [clangd] Fix Origin and MainFileOnly-ness for macros

kadircet added a comment. > the test.h in the patch description is missing a #define X. ah right, git descriptions omitting lines starting with `#` fixed it for include, but missed the define :face_palm: Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:383 +

[PATCH] D84297: [clangd] Fix Origin and MainFileOnly-ness for macros

This revision was automatically updated to reflect the committed changes. Closed by commit rGa69f9a8584f2: [clangd] Fix Origin and MainFileOnly-ness for macros (authored by kadircet). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D84297/new/ https://

[PATCH] D83759: [clangd] Fixes in lit tests

kadircet added inline comments. Comment at: clang-tools-extra/clangd/test/background-index.test:6 +# RUN: sed -e "s|DIRECTORY|%/t|" %/t/definition.jsonrpc > %/t/definition.jsonrpc.1 +# RUN: sed -i.bak -e "s|DIRECTORY|%/t|" %/t/compile_commands.json +# On Windows, we need the URI

[PATCH] D83759: [clangd] Fixes in lit tests

kadircet added inline comments. Comment at: clang-tools-extra/clangd/test/background-index.test:6 +# RUN: sed -e "s|DIRECTORY|%/t|" %/t/definition.jsonrpc > %/t/definition.jsonrpc.1 +# RUN: sed -i.bak -e "s|DIRECTORY|%/t|" %/t/compile_commands.json +# On Windows, we need the URI

[PATCH] D83759: [clangd] Fixes in lit tests

kadircet accepted this revision. kadircet added a comment. thanks, LGTM. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83759/new/ https://reviews.llvm.org/D83759 ___ cfe-commits mailing list cfe-commit

[PATCH] D84513: [clangd] Collect references for externally visible main-file symbols

kadircet marked an inline comment as done. kadircet added a comment. thanks, this looks good in general, but it would be nice to make sure we are not blowing the index memory and disk usage. could you grab some numbers for these two before/after this patch? Comment at: clang-

[PATCH] D84499: [clangd] Add more logs and attach tracers to remote index server routines

kadircet added inline comments. Comment at: clang-tools-extra/clangd/index/remote/Client.cpp:61 Callback(*Response); + ++Received; } shouldn't we increment this before the `continue` above ? (or rename this to `successful` rather than `received`

[PATCH] D84525: [clangd] Add marshalling code for all request types

kadircet added inline comments. Comment at: clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp:53 -clangd::FuzzyFindRequest -Marshaller::fromProtobuf(const FuzzyFindRequest *Request) { +namespace { +template nit: move anon namespace to the top

[PATCH] D84535: [clangd] Switch from EXPECT_TRUE to ASSERT_TRUE in remote marshalling tests

kadircet accepted this revision. kadircet added a comment. This revision is now accepted and ready to land. thanks LGTM! Comment at: clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp:64 *Serialized->mutable_location()->mutable_file_path() = std::string(); Des

[PATCH] D84525: [clangd] Add marshalling code for all request types

kadircet added inline comments. Comment at: clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp:111 + Req.IDs = std::move(*IDs); + Req.Filter = static_cast(Message->filter()); + if (Message->limit()) can you also add tests for these? (well actua

[PATCH] D84525: [clangd] Add marshalling code for all request types

kadircet added a comment. mostly LG. sorry for lots of nits, only two important bits are changing the {1} to {0} in logs, wrapping symbolid generations in `llvm::cantFail` and making sure `Deserialized` is exactly the same thing as `Request` in tests. feel free to ignore the rest (I should've m

[PATCH] D84513: [clangd] Collect references for externally visible main-file symbols

kadircet accepted this revision. kadircet added a comment. This revision is now accepted and ready to land. thanks! numbers and the patch LGTM. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D84513/new/ https://reviews.llvm.org/D84513

[PATCH] D84525: [clangd] Add marshalling code for all request types

kadircet accepted this revision. kadircet added a comment. This revision is now accepted and ready to land. thanks for bearing with me, LGTM! Comment at: clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp:277 + clangd::LookupRequest Request; + auto ID = llvm::cant

[PATCH] D84499: [clangd] Add more logs and attach tracers to remote index server routines

kadircet accepted this revision. kadircet added a comment. This revision is now accepted and ready to land. thanks, LGTM apart from templated-lambdas. Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:86 +unsigned FailedToSend = 0; +Index->lookup(Req,

[PATCH] D83224: [clangd] Move clang-tidy check modifications into ClangdServer

kadircet added a comment. In D83224#2133457 , @njames93 wrote: > Do you think there should be a hidden command line option to disable > disabling blacklisted checks, purely to prevent hindering attempts to fix > these problematic checks. I would expect

[PATCH] D84839: Add document outline symbols from unnamed contexts, e.g. extern "C".

kadircet added inline comments. Comment at: clang-tools-extra/clangd/FindSymbols.cpp:200 auto *ND = llvm::dyn_cast(D); -if (!ND) +if (!ND) { + // Traverse children of unnamed contexts, e.g. extern "C". this will result in traversal of other decl

[PATCH] D84919: [clangd] Be more explicit on testing the optional DefLoc in LocatedSymbol.

kadircet added inline comments. Comment at: clang-tools-extra/clangd/XRefs.cpp:430 Located.PreferredDeclaration = bool(Sym.Definition) ? DefLoc : DeclLoc; -Located.Definition = DefLoc; +if (Sym.Definition) + Located.Definition = DefLoc; this is

[PATCH] D84912: [clangd] findNearbyIdentifier(): fix the word search in the token stream.

kadircet accepted this revision. kadircet added a comment. This revision is now accepted and ready to land. thanks for catching this, LGTM! This might still end up traversing the whole file in non-existent cases, but I suppose that's an adventure for another day. I think we should also cherry-pi

[PATCH] D84912: [clangd] findNearbyIdentifier(): fix the word search in the token stream.

kadircet added a comment. oh and also, how did you notice the discrepancy? was it a random encounter while reading the code or did you notice a misbehaviour? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D84912/new/ https://reviews.llvm.org/D84912

[PATCH] D84894: [clangd] Implement Relations request for remote index

kadircet added a comment. thanks mostly nits, only annoying bit is usage of optionals and multi-logging but letting them be for now :D. also looks like there are some irrelevant changes, e.g. auto's in lambdas or formatting in protos, feel free to land an NFC change(without review) for those b

[PATCH] D84919: [clangd] Be more explicit on testing the optional DefLoc in LocatedSymbol.

kadircet added inline comments. Comment at: clang-tools-extra/clangd/unittests/XRefsTests.cpp:285 ::testing::Matcher Sym(std::string Name, Range Decl) { - return Sym(Name, Decl, llvm::None); + return Sym(Name, Decl, Decl); } hokein wrote: > kadircet wrote: >

[PATCH] D84894: [clangd] Implement Relations request for remote index

kadircet accepted this revision. kadircet added a comment. This revision is now accepted and ready to land. thanks, lgtm! Comment at: clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp:391 + auto Deserialized = ProtobufMarshaller.fromProtobuf(*Serialized); + ASSER

[PATCH] D84939: [clangd] Propagate remote index errors via Expected

kadircet added a comment. i think you might want to have an helper for creating stringerrors and use it in all places Comment at: clang-tools-extra/clangd/index/remote/Client.cpp:60 + Response.takeError()); +llvm::consumeError(Response.takeError());

[PATCH] D84919: [clangd] Be more explicit on testing the optional DefLoc in LocatedSymbol.

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

[PATCH] D77385: [clangd] Add index inspection helper tool

kadircet added a comment. Thanks for the patch! We already have one introspection tool called `dexp` have you give it a try? It can read both RIFF and YAML and allows you to run queries on the index. If it is not enough for your use case, can you describe it a little more? Maybe it would be ea

[PATCH] D77392: [clangd] Make signatureHelp work with stale preambles

kadircet created this revision. kadircet added a reviewer: sammccall. Herald added subscribers: cfe-commits, usaxena95, jfb, arphaman, mgrang, jkorous, MaskRay, javed.absar, ilya-biryukov. Herald added a project: clang. This is achieved by calculating newly added includes and implicitly parsing t

[PATCH] D77355: [clangd] show layout info when hovering on a class/field definition.

kadircet accepted this revision. kadircet marked an inline comment as done. kadircet added a comment. This revision is now accepted and ready to land. thanks! Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:69 struct Foo { - int [[b^ar]];

[PATCH] D77408: [clang] Annotate trivial getters and setters on hover.

kadircet accepted this revision. kadircet added a comment. This revision is now accepted and ready to land. LGTM, thanks! Comment at: clang-tools-extra/clangd/Hover.cpp:376 +llvm::Optional fieldName(const Expr *E) { + const auto *ReturnedMember = llvm::dyn_cast(E->IgnoreCasts(

[PATCH] D77456: [clangd] Parse `foo` in documentation comments and render as code.

kadircet accepted this revision. kadircet added a comment. This revision is now accepted and ready to land. LGTM, thanks! Regarding spaces between code and text chunks, are you suggesting we should print: Tests primality of`p` if so, i do believe having a space before the backtick vs not hav

[PATCH] D77456: [clangd] Parse `foo` in documentation comments and render as code.

kadircet added a comment. > But I'm wondering how good an idea this is for plaintext: "Returns 'foo' on > failure" is better than "Returns foo on failure", right? (With backticks > instead of single quotes, phab is eating my formatting) ah sorry missed that one. That's actually a good point, I

[PATCH] D77534: [clangd] DefineOutline: removes static token from static CXXMethodDecl

kadircet added a comment. thanks for the patch! this is quite similar to handling `kw_virtual` even handling multiple `static` keywords. Could you rather factor the `search for kw, generate an edit to delete specifier` part into a `DelKeyword` lambda similar to `DelAttr` and call it with kw_st

[PATCH] D77570: [clang][CodeComplete] Dont perform fallback completion for incomplete member ref

kadircet created this revision. kadircet added a reviewer: sammccall. Herald added a project: clang. Herald added a subscriber: cfe-commits. Clang performs expression based completion whenever it can't figure out base of a member reference expression. It might be quite confusing in cases like inco

[PATCH] D77570: [clang][CodeComplete] Dont perform fallback completion for incomplete member ref

This revision was automatically updated to reflect the committed changes. Closed by commit rG0731132888a0: [clang][CodeComplete] Dont perform fallback completion for incomplete member ref (authored by kadircet). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.ll

[PATCH] D76725: [clangd] Build ASTs only with fresh preambles or after building a new preamble

kadircet updated this revision to Diff 255425. kadircet marked 10 inline comments as done. kadircet added a comment. - Address comments and rebase Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D76725/new/ https://reviews.llvm.org/D76725 Files: cl

[PATCH] D76725: [clangd] Build ASTs only with fresh preambles or after building a new preamble

kadircet marked an inline comment as done. kadircet added inline comments. Comment at: clang-tools-extra/clangd/TUScheduler.cpp:224 + std::vector CIDiags, WantDiagnostics WantDiags) { // Make possibly expensive copy while not holding the lock. +Request Req =

[PATCH] D77309: [clangd] Get rid of ASTWorker::getCurrentFileInputs

kadircet updated this revision to Diff 255435. kadircet added a comment. - Rebase Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77309/new/ https://reviews.llvm.org/D77309 Files: clang-tools-extra/clangd/TUScheduler.cpp Index: clang-tools-extra/

[PATCH] D77534: [clangd] DefineOutline: removes static token from static CXXMethodDecl

kadircet added inline comments. Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp:242 - if (FD->isVirtualAsWritten()) { -SourceRange SpecRange{FD->getBeginLoc(), FD->getLocation()}; -bool HasErrors = true; - -// Clang allows duplicating virtual

[PATCH] D76125: [clangd] Decouple preambleworker from astworker, NFCI

This revision was automatically updated to reflect the committed changes. Closed by commit rG276a95bdf27c: [clangd] Decouple preambleworker from astworker, NFCI (authored by kadircet). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D76125/new/ https:/

[PATCH] D76304: [clangd] Update TUStatus api to accommodate preamble thread

This revision was automatically updated to reflect the committed changes. Closed by commit rG6b85032c95be: [clangd] Update TUStatus api to accommodate preamble thread (authored by kadircet). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D76304/new/ h

[PATCH] D76725: [clangd] Build ASTs only with fresh preambles or after building a new preamble

This revision was automatically updated to reflect the committed changes. Closed by commit rGc31367e95ce1: [clangd] Build ASTs only with fresh preambles or after building a new preamble (authored by kadircet). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm

[PATCH] D77534: [clangd] DefineOutline: removes static token from static CXXMethodDecl

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

[PATCH] D77615: [Syntax] Merge overlapping top-level macros in TokenBuffer

kadircet accepted this revision. kadircet added a comment. This revision is now accepted and ready to land. LGTM, thanks! Comment at: clang/lib/Tooling/Syntax/Tokens.cpp:452 + +// The *last* token of the macro reference is in the main file for A and B. +if (Range.getEnd

[PATCH] D77614: [Syntax] Simplify TokenCollector::Builder, use captured expansion bounds. NFC

kadircet accepted this revision. kadircet 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/D77614/new/ https://reviews.llvm.org/D77614 __

[PATCH] D77614: [Syntax] Simplify TokenCollector::Builder, use captured expansion bounds. NFC

kadircet added inline comments. Comment at: clang/lib/Tooling/Syntax/Tokens.cpp:560 + SpelledTokens[NextSpelled].location() < Target) { + // If we know of mapping bounds at [Next, KnownEnd] (e.g. macro expansion) + // then we want to partition our (empty) map

[PATCH] D77614: [Syntax] Simplify TokenCollector::Builder, use captured expansion bounds. NFC

kadircet added a comment. still LG Comment at: clang/lib/Tooling/Syntax/Tokens.cpp:588 +auto &NextSpelled = this->NextSpelled[File]; + +if (Tok.location().isFileID()) { sammccall wrote: > kadircet wrote: > > nit: maybe create mapping here and increment

[PATCH] D77309: [clangd] Get rid of ASTWorker::getCurrentFileInputs

kadircet updated this revision to Diff 255632. kadircet marked an inline comment as done. kadircet added a comment. - Address comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77309/new/ https://reviews.llvm.org/D77309 Files: clang-tools-ex

[PATCH] D77309: [clangd] Get rid of ASTWorker::getCurrentFileInputs

This revision was automatically updated to reflect the committed changes. Closed by commit rG4ac7b805b7c4: [clangd] Get rid of ASTWorker::getCurrentFileInputs (authored by kadircet). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77309/new/ https://r

[PATCH] D77644: [clangd] Handle additional includes while parsing ASTs

kadircet created this revision. kadircet added a reviewer: sammccall. Herald added subscribers: cfe-commits, usaxena95, arphaman, mgrang, jkorous, MaskRay, ilya-biryukov. Herald added a project: clang. Depends on D77392 . Enables building ASTs with stale preamble

[PATCH] D77664: [clangd] Fix broken assertion

kadircet created this revision. kadircet added a reviewer: sammccall. Herald added subscribers: cfe-commits, usaxena95, arphaman, jkorous, MaskRay, javed.absar, ilya-biryukov. Herald added a project: clang. This assertion was bad. It will show up once we start running preamble thread async. Think

[PATCH] D77664: [clangd] Fix broken assertion

kadircet updated this revision to Diff 255746. kadircet added a comment. - Delete the assertion too Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77664/new/ https://reviews.llvm.org/D77664 Files: clang-tools-extra/clangd/TUScheduler.cpp Index:

[PATCH] D77669: [clangd] Run TUStatus test in sync mode to make output deterministic

kadircet created this revision. kadircet added a reviewer: sammccall. Herald added subscribers: cfe-commits, usaxena95, arphaman, mgrang, jkorous, MaskRay, javed.absar, ilya-biryukov. Herald added a project: clang. Currently it doesn't matter because we run PreambleThread in sync mode. Once we st

[PATCH] D77671: [clangd] Destroy context before resetting CurrentReq

kadircet created this revision. kadircet added a reviewer: sammccall. Herald added subscribers: cfe-commits, usaxena95, arphaman, jkorous, MaskRay, javed.absar, ilya-biryukov. Herald added a project: clang. Our tests stash callbacks into request context and rely on it being invoked before threads

[PATCH] D77644: [clangd] Handle additional includes while parsing ASTs

kadircet planned changes to this revision. kadircet added a comment. this doesn't handle source locations correctly in presence of deleted headers. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77644/new/ https://reviews.llvm.org/D77644

[PATCH] D77669: [clangd] Update TUStatus to handle async PreambleThread

kadircet added inline comments. Comment at: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp:849 + auto Opts = ClangdServer::optsForTest(); + Opts.AsyncThreadsCount = 0; + ClangdServer Server(CDB, FS, Opts, &CaptureTUStatus); sammccall wrote: > This see

[PATCH] D77669: [clangd] Update TUStatus to handle async PreambleThread

kadircet updated this revision to Diff 255914. kadircet marked 4 inline comments as done. kadircet added a comment. - Assert on the execution order instead with simplifications. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77669/new/ https://revie

[PATCH] D77664: [clangd] Fix broken assertion

This revision was automatically updated to reflect the committed changes. Closed by commit rG130dbf63ff12: [clangd] Fix broken assertion (authored by kadircet). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77664/new/ https://reviews.llvm.org/D77664

[PATCH] D77671: [clangd] Destroy context before resetting CurrentReq

This revision was automatically updated to reflect the committed changes. Closed by commit rG2a6eedbb51fd: [clangd] Destroy context before resetting CurrentReq (authored by kadircet). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77671/new/ https://

[PATCH] D77717: [clang][index] index the missing LabelDecl in libindex.

kadircet added a comment. we should also be handling `LabelDecl`s in clang/lib/Index/IndexDecl.cpp Comment at: clang/lib/Index/IndexBody.cpp:145 +if (auto *LabelDecl = Goto->getLabel()) + IndexCtx.handleReference(LabelDecl, Goto->getLabelLoc(), Parent, ParentDC, +

[PATCH] D77732: [clangd] Shard preamble symbols in dynamic index

kadircet created this revision. kadircet added a reviewer: sammccall. Herald added subscribers: cfe-commits, usaxena95, arphaman, jkorous, MaskRay, ilya-biryukov. Herald added a project: clang. This reduces memory usage by dynamic index from more than 400MB to 32MB when all files in clang-tools-e

[PATCH] D77717: [clang][index] index the missing LabelDecl in libindex.

kadircet added inline comments. Comment at: clang/lib/Index/IndexBody.cpp:152 +if (auto *LabelDecl = Label->getDecl()) + IndexCtx.handleReference(LabelDecl, Label->getIdentLoc(), Parent, + ParentDC, argh, looks like Recursiv

[PATCH] D77766: [clangd] Set up machinery for gtests of ClangdLSPServer.

kadircet added a comment. LGTM, mostly nits. Unfortunately I don't see any way to make it simpler :/ Comment at: clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp:104 + +MATCHER_P(DiagMessage, M, "") { + if (const auto *O = arg.getAsObject()) { nit:

[PATCH] D77392: [WIP][clangd] Make signatureHelp work with stale preambles

kadircet updated this revision to Diff 256359. kadircet added a comment. Herald added a subscriber: mgorny. - Encapsulate logic into `PreamblePatch` Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77392/new/ https://reviews.llvm.org/D77392 Files:

[PATCH] D77392: [WIP][clangd] Make signatureHelp work with stale preambles

kadircet marked 3 inline comments as done. kadircet added inline comments. Comment at: clang-tools-extra/clangd/CodeComplete.cpp:1076 + if (PatchAdditionalIncludes) { +for (const auto &Inc : newIncludes( + Input.Preamble.LexedIncludes, sammccall

[PATCH] D77766: [clangd] Set up machinery for gtests of ClangdLSPServer.

kadircet added inline comments. Comment at: clang-tools-extra/clangd/unittests/LSPClient.cpp:65 + + void reply(llvm::json::Value ID, + llvm::Expected V) override { sammccall wrote: > kadircet wrote: > > put methods before fields > Done - I don't care

[PATCH] D77847: [clangd] Rebuild dependent files when a header is saved.

kadircet added a comment. should we also have a unittest for checking ast caching works as expected? ClangdServer::getUsedBytesPerFile should allow us to do that. We can set cache size to one, send 3 updates in respective order to foo, bar and baz, we record usedbytesperfile. Then we perform ad

[PATCH] D77847: [clangd] Rebuild dependent files when a header is saved.

kadircet accepted this revision. kadircet added a comment. This revision is now accepted and ready to land. Thanks, LGTM! In D77847#1975940 , @sammccall wrote: > In D77847#1974126 , @kadircet wrote: > > > should we

[PATCH] D77944: [clangd][test] Provide registered targets to lit tests

kadircet created this revision. kadircet added a reviewer: sammccall. Herald added subscribers: cfe-commits, usaxena95, arphaman, jkorous, MaskRay, ilya-biryukov. Herald added a project: clang. We had tests in clangd (target_info.test) that got enabled only on systems that know about x86. But the

[PATCH] D77847: [clangd] Rebuild dependent files when a header is saved.

kadircet added a comment. In D77847#1976077 , @sammccall wrote: > OK, I misunderstood what we're testing, please tell me if I got it right this > time :-) > > Plausible bad behavior: we send a no-op change to a relatively inactive file. > TUScheduler bui

[PATCH] D77942: [Preamble] Invalidate preamble when missing headers become present.

kadircet added a comment. It is unfortunate that we are testing `PrecompiledPreamble` through clangd rather than its own unittest :/ Comment at: clang/lib/Frontend/PrecompiledPreamble.cpp:101 + + // We always receive FileNotFound followed by InclusionDirective. + // We want

[PATCH] D77944: [clangd][test] Provide registered targets to lit tests

This revision was automatically updated to reflect the committed changes. kadircet marked an inline comment as done. Closed by commit rGd34a91a10f7a: [clangd][test] Provide registered targets to lit tests (authored by kadircet). Changed prior to commit: https://reviews.llvm.org/D77944?vs=256774

[PATCH] D77955: [clangd] Add target_info test

kadircet created this revision. Herald added subscribers: cfe-commits, phosek, usaxena95, arphaman, jkorous, MaskRay, ilya-biryukov. Herald added a project: clang. Repository: rG LLVM Github Monorepo https://reviews.llvm.org/D77955 Files: clang-tools-extra/clangd/test/target_info.test Ind

[PATCH] D77955: [clangd] Add target_info test

kadircet updated this revision to Diff 256841. kadircet added a comment. - Only test for arm Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77955/new/ https://reviews.llvm.org/D77955 Files: clang-tools-extra/clangd/test/target_info.test Index:

[PATCH] D77947: [clangd] Send the correct error code when cancelling requests.

kadircet accepted this revision. kadircet added a comment. This revision is now accepted and ready to land. Thanks for remembering this :D LGTM! Comment at: clang-tools-extra/clangd/JSONTransport.cpp:28 + [&](const CancelledError &C) -> llvm::Error { +switc

[PATCH] D77732: [clangd] Shard preamble symbols in dynamic index

kadircet marked an inline comment as done. kadircet added inline comments. Comment at: clang-tools-extra/clangd/index/FileIndex.cpp:97 +/// paths used by \p FileSyms. +void shardSlabToFiles(const SymbolSlab &Syms, const RelationSlab &Rels, + FileSymbols &File

[PATCH] D77955: [clangd] Add target_info test

kadircet abandoned this revision. kadircet added a comment. landed as 101a69d71b93f901561621508ed36b187e594d91 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77955/new/ https://r

[PATCH] D77669: [clangd] Update TUStatus test to handle async PreambleThread

kadircet updated this revision to Diff 256875. kadircet marked 6 inline comments as done. kadircet added a comment. - Record actions for each thread separately. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77669/new/ https://reviews.llvm.org/D7766

[PATCH] D77669: [clangd] Update TUStatus test to handle async PreambleThread

kadircet added a comment. going with separately recording each thread. that way we can also test building of diagnostics. Comment at: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp:849 + auto Opts = ClangdServer::optsForTest(); + Opts.AsyncThreadsCount = 0; + Clang

[PATCH] D77669: [clangd] Update TUStatus test to handle async PreambleThread

This revision was automatically updated to reflect the committed changes. Closed by commit rG48d851a92e98: [clangd] Update TUStatus test to handle async PreambleThread (authored by kadircet). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77669/new/

[PATCH] D77732: [clangd] Shard preamble symbols in dynamic index

kadircet updated this revision to Diff 257003. kadircet marked 4 inline comments as done. kadircet added a comment. - Unify sharding logic in BackgroundIndex and FileIndex. - Make sure relations have valid subjects. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://revie

[PATCH] D77732: [clangd] Shard preamble symbols in dynamic index

kadircet added inline comments. Comment at: clang-tools-extra/clangd/index/FileIndex.cpp:97 +/// paths used by \p FileSyms. +void shardSlabToFiles(const SymbolSlab &Syms, const RelationSlab &Rels, + FileSymbols &FileSyms, llvm::StringRef HintPath) { -

[PATCH] D77732: [clangd] Shard preamble symbols in dynamic index

kadircet marked an inline comment as done. kadircet added inline comments. Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:286 + // ND is the canonical (i.e. first) declaration. If it's in the main file + // (which is not a header), then no public declaration w

[PATCH] D78038: [clangd] WIP: fix several bugs relating to include insertion

kadircet added inline comments. Comment at: clang/lib/Lex/Lexer.cpp:2749 +// most useful answer is "yes, this file has a header guard". +if (!ConditionalStack.empty()) + MIOpt.ExitTopLevelConditional(); I think we should put this behind a PPOpt to ma

[PATCH] D78048: [clangd] Add tests that no-op changes are cheap

kadircet accepted this revision. kadircet added a comment. This revision is now accepted and ready to land. thanks for doing this LGTM! Comment at: clang-tools-extra/clangd/unittests/ClangdTests.cpp:501 +MATCHER_P4(Stats, Name, UsesMemory, PreambleBuilds, ASTBuilds, "") { + re

[PATCH] D77732: [clangd] Shard preamble symbols in dynamic index

kadircet updated this revision to Diff 257242. kadircet marked 7 inline comments as done. kadircet added a comment. - Address comments - Merge slab generations into a single member that returns an `IndexFileIn`. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.l

[PATCH] D77732: [clangd] Shard preamble symbols in dynamic index

kadircet added inline comments. Comment at: clang-tools-extra/clangd/index/FileIndex.h:165 + /// Returns absolute paths for all files that has a shard. + std::vector getAllFiles() const; + sammccall wrote: > I do find it a little weird not to expose the map-str

[PATCH] D77732: [clangd] Shard preamble symbols in dynamic index

kadircet updated this revision to Diff 257607. kadircet added a comment. - Add tests for sharding logic and preamble overwrite Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77732/new/ https://reviews.llvm.org/D77732 Files: clang-tools-extra/clan

[PATCH] D77732: [clangd] Shard preamble symbols in dynamic index

This revision was automatically updated to reflect the committed changes. Closed by commit rGdffa9dfbda56: [clangd] Shard preamble symbols in dynamic index (authored by kadircet). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77732/new/ https://revi

[PATCH] D78181: [clangd] Fix a crash for accessing a null field decl returned by findExplicitReferences.

kadircet added inline comments. Comment at: clang-tools-extra/clangd/FindTarget.cpp:702 continue; +if (!D.getField()) + continue; thanks for fixing this ! But I suppose we should rather return a result with empty `TargetDecl` as this

[PATCH] D78235: [clangd] Store ppdirective in Inclusion

kadircet created this revision. kadircet added a reviewer: sammccall. Herald added subscribers: cfe-commits, usaxena95, arphaman, jkorous, MaskRay, ilya-biryukov. Herald added a project: clang. This will enable PreamblePatching proposed in D77392 craft a more in

[PATCH] D81173: [clangd] Change ParseInputs to store FSProvider rather than VFS

kadircet created this revision. kadircet added a reviewer: sammccall. Herald added subscribers: cfe-commits, usaxena95, arphaman, jkorous, MaskRay, javed.absar, ilya-biryukov. Herald added a project: clang. This ensures ParseInputs provides a read-only access to FS. Repository: rG LLVM Github

[PATCH] D80900: [clangd] Use different FS in PreambleThread

kadircet added a comment. In D80900#2071207 , @sammccall wrote: > That makes sense. However I don't think that contract is conceptually that > great, having the ideas of "threadsafe FS" and "maybe-context-aware FS" > coupled together is... weird. The ben

<    7   8   9   10   11   12   13   14   15   16   >