[PATCH] D88417: [clangd] Record memory usages after each notification

kadircet added a comment. > What about something like a 5 minute throttle, but have ClangdLSPServer's > constructor set the timestamp to now+1 minute? (Without profiling) SGTM. Note that this means we can't easily test this in LSP layer anymore. (We've got couple of components depending on time

[PATCH] D88417: [clangd] Record memory usages after each notification

kadircet updated this revision to Diff 297173. kadircet marked 2 inline comments as done. kadircet added a comment. - Implement periodic profiling Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D88417/new/ https://reviews.llvm.org/D88417 Files: cl

[PATCH] D89135: [clangd] Stop capturing trace args if the tracer doesn't need them.

kadircet accepted this revision. kadircet added inline comments. This revision is now accepted and ready to land. Comment at: clang-tools-extra/clangd/support/Trace.cpp:91 public: -JSONSpan(JSONTracer *Tracer, llvm::StringRef Name, llvm::json::Object *Args) +llvm::jso

[PATCH] D88204: [clangd] Drop path suffix mapping for std symbols

kadircet marked an inline comment as done. kadircet added a comment. > We don't have an alternative database for these. Maybe we should keep the > mechanism but stop using it for the standard library? What do you think? Yes i think you are right. Just dropping STL mappings from the list. Repos

[PATCH] D88204: [clangd] Drop path suffix mapping for std symbols

kadircet updated this revision to Diff 297528. kadircet added a comment. - Only drop STL mapping instead of getting rid of suffix mapping completely. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D88204/new/ https://reviews.llvm.org/D88204 Files:

[PATCH] D88413: [clangd] Add a metric for tracking memory usage

kadircet updated this revision to Diff 297542. kadircet marked an inline comment as done. kadircet added a comment. - Make traversal a free function and take metric to populate as a parameter. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D88413/new/

[PATCH] D89135: [clangd] Stop capturing trace args if the tracer doesn't need them.

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

[PATCH] D88417: [clangd] Record memory usages after each notification

kadircet updated this revision to Diff 297565. kadircet marked 5 inline comments as done. kadircet added a comment. - Separate profiling and exporting into 2 functions - Rebase Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D88417/new/ https://review

[PATCH] D88417: [clangd] Record memory usages after each notification

kadircet updated this revision to Diff 297568. kadircet marked an inline comment as done. kadircet added a comment. - Update stale comment Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D88417/new/ https://reviews.llvm.org/D88417 Files: clang-tool

[PATCH] D88411: [clangd] Introduce MemoryTrees

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGf9317f7bf6bd: [clangd] Introduce MemoryTrees (authored by kadircet). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://revi

[PATCH] D88414: [clangd] Introduce memory dumping to FileIndex, FileSymbols and BackgroundIndex

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGa74d59494861: [clangd] Introduce memory dumping to FileIndex, FileSymbols and BackgroundIndex (authored by kadircet). Repository: rG LLVM Github M

[PATCH] D88413: [clangd] Add a metric for tracking memory usage

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGc9d2876da95c: [clangd] Add a metric for tracking memory usage (authored by kadircet). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTI

[PATCH] D88417: [clangd] Record memory usages after each notification

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG35871fde55ac: [clangd] Record memory usages after each notification (authored by kadircet). Repository: rG LLVM Github Monorepo CHANGES SINCE LAS

[PATCH] D88415: [clangd] Introduce memory usage dumping to TUScheduler, for Preambles and ASTCache

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG23a53301c545: [clangd] Introduce memory usage dumping to TUScheduler, for Preambles and… (authored by kadircet). Repository: rG LLVM Github Monore

[PATCH] D89277: [clangd] Add $/dumpMemoryTree LSP extension

kadircet created this revision. kadircet added a reviewer: sammccall. Herald added subscribers: cfe-commits, usaxena95, arphaman. Herald added a project: clang. kadircet requested review of this revision. Herald added subscribers: MaskRay, ilya-biryukov. Performs a detailed profiling of clangd lsp

[PATCH] D89277: [clangd] Add $/dumpMemoryTree LSP extension

kadircet added a comment. In D89277#2326376 , @nridge wrote: > This is neat! Are there plans to add vscode client support to invoke this? Yes, since we control the plugin in vscode we should have a way to invoke it there. (we already have support for `sw

[PATCH] D89307: [clangd] Disable extract variable for RHS of assignments

kadircet created this revision. kadircet added a reviewer: adamcz. Herald added subscribers: cfe-commits, usaxena95, arphaman. Herald added a project: clang. kadircet requested review of this revision. Herald added subscribers: MaskRay, ilya-biryukov. Repository: rG LLVM Github Monorepo https:/

[PATCH] D89307: [clangd] Disable extract variable for RHS of assignments

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG82a71822a54d: [clangd] Disable extract variable for RHS of assignments (authored by kadircet). Repository: rG LLVM Github Monorepo CHANGES SINCE

[PATCH] D89277: [clangd] Add $/dumpMemoryTree LSP extension

kadircet added a comment. In D89277#2329947 , @sammccall wrote: > (sorry out today and haven't looked at code yet) no worries it is a prototype, I wouldn't spend time looking at the implementation until we agree on the interaction :D OTHO, checking out t

[PATCH] D89297: [clangd] Add a TestWorkspace utility

kadircet added a comment. This looks like a useful infra to have indeed, we currently don't have an easy way to setup a testcase that would require interactions between multiple files (e.g. a workspace), as you've noticed while working on callhierarchy tests (sorry for that). A couple of sugge

[PATCH] D89277: [clangd] Add $/dumpMemoryTree LSP extension

kadircet updated this revision to Diff 298460. kadircet added a comment. - As discussed offline moving with compact version of the output, while preserving names starting with an `_` to be leaves. This is also ready for an implementation-wise review now. Repository: rG LLVM Github Monorepo

[PATCH] D89277: [clangd] Add $/dumpMemoryTree LSP extension

kadircet updated this revision to Diff 298578. kadircet marked 5 inline comments as done. kadircet added a comment. - Add memoryTreeProvider to initialization params - Move serialization logic to Protocol.h and return MemoryTree directly. - Drop `dump` from method name. Repository: rG LLVM Git

[PATCH] D89277: [clangd] Add $/dumpMemoryTree LSP extension

kadircet updated this revision to Diff 298644. kadircet added a comment. s/memoryTree/memoryUsage/g Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D89277/new/ https://reviews.llvm.org/D89277 Files: clang-tools-extra/clangd/ClangdLSPServer.cpp cl

[PATCH] D91868: [clangd] Mention when CXXThis is implicit in exposed AST.

kadircet added a comment. This looks like an improvement to me as well, thanks! Comment at: clang-tools-extra/clangd/DumpAST.cpp:233 return CCO->getConstructor()->getNameAsString(); +if (const auto *CTE = dyn_cast(S)) { + bool Const = CTE->getType()->getPointeeT

[PATCH] D91859: [clangd] Fix shared-lib builds

kadircet added a subscriber: nridge. kadircet added a comment. > I don't think that's how CMake works, the whole CMakeLists tree is parsed > before anything is compiled, so it shouldn't race like that. That was also my mental model, but it looks like `include_directories` statement within the `

[PATCH] D91859: [clangd] Fix shared-lib builds

kadircet updated this revision to Diff 306830. 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/D91859/new/ https://reviews.llvm.org/D91859 Files: clang-tools-ex

[PATCH] D91868: [clangd] Mention when CXXThis is implicit in exposed AST.

kadircet accepted this revision. kadircet added inline comments. This revision is now accepted and ready to land. Comment at: clang-tools-extra/clangd/DumpAST.cpp:233 return CCO->getConstructor()->getNameAsString(); +if (const auto *CTE = dyn_cast(S)) { + bool Con

[PATCH] D91124: [clangd] Call hierarchy (ClangdLSPServer layer)

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/D91124/new/ https://reviews.llvm.org/D91124

[PATCH] D91123: [clangd] Call hierarchy (ClangdServer layer)

kadircet accepted this revision. kadircet added inline comments. This revision is now accepted and ready to land. Comment at: clang-tools-extra/clangd/ClangdServer.cpp:695 +Callback>> CB) { + CB(clangd::incomingCalls(Item, Index)); +} nridge wrote: > kadirce

[PATCH] D91860: [clangd] Move remote-index dependency from clangDaemon to ClangdMain

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

[PATCH] D91860: [clangd] Move remote-index dependency from clangDaemon to ClangdMain

kadircet updated this revision to Diff 306840. kadircet added a comment. - Merge with https://reviews.llvm.org/D90751 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D91860/new/ https://reviews.llvm.org/D91860 Files: clang-tools-extra/clangd/CMakeL

[PATCH] D90751: [clangd] Use ProjectAwareIndex

kadircet abandoned this revision. kadircet added a comment. Merged with https://reviews.llvm.org/D91860 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D90751/new/ https://reviews.llvm.org/D90751 ___ cfe-co

[PATCH] D91122: [clangd] Call hierarchy (XRefs layer, incoming calls)

kadircet added inline comments. Comment at: clang-tools-extra/clangd/XRefs.h:109 +/// Get call hierarchy information at \p Pos. +llvm::Optional> +prepareCallHierarchy(ParsedAST &AST, Position Pos, const SymbolIndex *Index, nridge wrote: > kadircet wrote: > > nrid

[PATCH] D90750: [clangd] Introduce ProjectAwareIndex

kadircet updated this revision to Diff 306924. 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/D90750/new/ https://reviews.llvm.org/D90750 Files: clang-tools-ex

[PATCH] D91859: [clangd] Fix shared-lib builds

kadircet added a comment. In D91859#2409743 , @nridge wrote: > I applied the patch locally and it fixes most of the linker errors but I'm > still seeing one: > > [6/393] Linking CXX executable bin/clangd-index-server > FAILED: bin/clangd-index-server

[PATCH] D90748: [clangd] Introduce config parsing for External blocks

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG359e2f988dc5: [clangd] Introduce config parsing for External blocks (authored by kadircet). Repository: rG LLVM Github Monorepo CHANGES SINCE LAS

[PATCH] D90749: [clangd] Introduce config compilation for External blocks

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGc9776c8d4ef7: [clangd] Introduce config compilation for External blocks (authored by kadircet). Changed prior to commit: https://reviews.llvm.org/

[PATCH] D90750: [clangd] Introduce ProjectAwareIndex

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG067ffbfe6018: [clangd] Introduce ProjectAwareIndex (authored by kadircet). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https:

[PATCH] D91860: [clangd] Move remote-index dependency from clangDaemon to ClangdMain

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGcab313680703: [clangd] Use ProjectAwareIndex in ClangdMain (authored by kadircet). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D91941: [clangd] Use WorkScheduler.run() in ClangdServer::resolveTypeHierarchy()

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

[PATCH] D91947: [clangd] testPath's final result agrees with the passed in Style

kadircet created this revision. kadircet added reviewers: hokein, sammccall. Herald added subscribers: cfe-commits, usaxena95, arphaman. Herald added a project: clang. kadircet requested review of this revision. Herald added subscribers: MaskRay, ilya-biryukov. This was confusing, as testRoot on w

[PATCH] D91122: [clangd] Call hierarchy (XRefs layer, incoming calls)

kadircet accepted this revision. kadircet added a comment. This revision is now accepted and ready to land. This looks great! Thanks a lot for bearing with me and doing all of this! Comment at: clang-tools-extra/clangd/XRefs.cpp:1613 +if (auto CHI = declToCallHierarchyItem(

[PATCH] D91947: [clangd] testPath's final result agrees with the passed in Style

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG8cec8de2a4e6: [clangd] testPath's final result agrees with the passed in Style (authored by kadircet). Repository: rG LLVM Github Monorepo CHANGE

[PATCH] D91930: [clangd] Implement textDocument/codeLens

kadircet added a comment. Thanks a lot for working on improving clangd! Can you also give a high-level overview of what kind of functionality you are providing here? Looks like there's a lot going on here, and it would be nice to know what you are attempting to do, rather than inferring that fr

[PATCH] D91966: [clangd] AddUsing: Used spelled text instead of type name.

kadircet added inline comments. Comment at: clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp:268 + SpelledTokens->back()); + llvm::StringRef NameRef = SpelledRange.text(SM); + what about saving the full rang

[PATCH] D91122: [clangd] Call hierarchy (XRefs layer, incoming calls)

kadircet added inline comments. Comment at: clang-tools-extra/clangd/unittests/CallHierarchyTests.cpp:86 + + auto IncomingLevel3 = incomingCalls(IncomingLevel2[0].from, Index.get()); + EXPECT_THAT(IncomingLevel3, the order of elements in `IncomingLevel2` is not

[PATCH] D92000: [clangd] Collect main file refs by default

kadircet added a comment. i remember discussing this in the past, and having "negative" feelings about it. I couldn't find the discussion on the issues page tho, could you toss me the link if you can find it? (it might've been an offline discussion as well, sorry if that's the case for not dump

[PATCH] D92000: [clangd] Collect main file refs by default

kadircet added a comment. NVM, found the summary https://github.com/clangd/clangd/issues/162#issuecomment-653981038 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D92000/new/ https://reviews.llvm.org/D92000 _

[PATCH] D92009: [clangd] Sort results of incomingCalls request by container name

kadircet accepted this revision. kadircet added a comment. This revision is now accepted and ready to land. Thanks! Comment at: clang-tools-extra/clangd/unittests/CallHierarchyTests.cpp:79 auto IncomingLevel2 = incomingCalls(IncomingLevel1[0].from, Index.get()); - EXPECT_TH

[PATCH] D91859: [clangd] Fix shared-lib builds

kadircet updated this revision to Diff 307281. kadircet added a comment. Herald added a project: LLVM. Herald added a subscriber: llvm-commits. - Link protobuf and grpc++ publicly to generated targets Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D91

[PATCH] D91859: [clangd] Fix shared-lib builds

This revision was automatically updated to reflect the committed changes. Closed by commit rGf726101b6240: [clangd] Fix shared-lib builds (authored by kadircet). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D91859/new/ https://reviews.llvm.org/D9185

[PATCH] D91966: [clangd] AddUsing: Used spelled text instead of type name.

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/refactor/tweaks/AddUsing.cpp:268 + SpelledTokens->back()); + llvm::S

[PATCH] D92041: Add hover info for `this` expr

kadircet added a comment. Thanks, this sounds like a sensible idea. I got a few suggestions for the implementation though. Comment at: clang-tools-extra/clangd/Hover.cpp:610 +/// Generate a \p Hover object given the \p this pointer. +HoverInfo getHoverContents(const CXXThisExp

[PATCH] D92012: [clangd][query-driver] Extract target

kadircet added inline comments. Comment at: clang-tools-extra/clangd/QueryDriverDatabase.cpp:212 + if (!Target.empty()) { +Cmd.CommandLine.push_back("-target"); +Cmd.CommandLine.push_back(Target); sammccall wrote: > `clang -target foo test.cc` seems to b

[PATCH] D92053: [clangd] Addusing tweak: find insertion point after definition

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/refactor/tweaks/AddUsing.cpp:158 + +if (MustInsertAfterLoc.isValid() && +SM.isBeforeInTranslationUnit(U->get

[PATCH] D92041: Add hover info for `this` expr

kadircet added inline comments. Comment at: clang-tools-extra/clangd/Hover.cpp:610 +/// Generate a \p Hover object given the \p this pointer. +HoverInfo getHoverContents(const CXXThisExpr *CTE, const SymbolIndex *Index) { + const NamedDecl *D = CTE->getType()->getPointeeType()->

[PATCH] D92077: [clangd] Avoid type hierarchy crash on incomplete type

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/D92077/new/ https://reviews.llvm.org/D92077

[PATCH] D92000: [clangd] Collect main file refs by default

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/tool/ClangdMain.cpp:492 desc("Store references to main-file-only symbols in the index"), -init(false), +in

[PATCH] D91930: [clangd] Implement textDocument/codeLens

kadircet added a comment. Thanks for the explanations. I've also had some discussions with @sammccall about this and would like to summarize them. There are two important things to keep in mind: - Collection is performed for the whole document. - Resolve requests cannot fail. (there's no `null`

[PATCH] D92012: [clangd][query-driver] Extract target

kadircet added inline comments. Comment at: clang-tools-extra/clangd/QueryDriverDatabase.cpp:67 + +bool isValidTarget(llvm::StringRef Triple) { + std::shared_ptr TargetOpts(new TargetOptions); i think you can just do `TargetRegistry::lookupTarget`

[PATCH] D92041: [clangd] Add hover info for `this` expr

kadircet added inline comments. Comment at: clang-tools-extra/clangd/Hover.cpp:610 +/// Generate a \p Hover object given the \p this pointer. +HoverInfo getHoverContents(const CXXThisExpr *CTE, const SymbolIndex *Index) { + const NamedDecl *D = CTE->getType()->getPointeeType()->

[PATCH] D92000: [clangd] Collect main file refs by default

kadircet added a comment. nope, as it doesn't change the serialization format. but any existing shards won't have refs from the main file, they'll accumulate over time as sources gets modified. (i don't think it is worth bumping the version to invalidate existing shards, if a user cares enough

[PATCH] D92012: [clangd][query-driver] Extract target

kadircet added inline comments. Comment at: clang-tools-extra/clangd/QueryDriverDatabase.cpp:344 +} +return Cmd; } ofc we don't need it. but the thing is we are returning an `Optional` hence the return statement needs to invoke a constructor for `Opt

[PATCH] D92012: [clangd][query-driver] Extract target

kadircet added inline comments. Comment at: clang-tools-extra/clangd/QueryDriverDatabase.cpp:344 +} +return Cmd; } kadircet wrote: > ofc we don't need it. but the thing is we are returning an > `Optional` hence the return statement needs to invoke a

[PATCH] D92157: [clangd] Add language metrics for recovery AST usage.

kadircet added inline comments. Comment at: clang-tools-extra/clangd/Selection.cpp:51 // Measure the fraction of selections that were enabled by recovery AST. -void recordMetrics(const SelectionTree &S) { +void recordMetrics(const SelectionTree &S, const ASTContext &AST) { st

[PATCH] D92041: [clangd] Add hover info for `this` expr

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/Hover.cpp:644 +const NamedDecl *D = CTE->getType()->getPointeeType()->getAsTagDecl(); +HI = getHoverContents(D,

[PATCH] D92198: [clangd] Implement remote index handshake

kadircet added a comment. Haven't checked the details but is there a specific reason for implementing a custom protocol rather than making use of `NotifyOnStateChange` (https://grpc.github.io/grpc/cpp/classgrpc_1_1_channel_interface.html) or even `WaitForStateChange` if we really want to block

[PATCH] D92041: [clangd] Add hover info for `this` expr

kadircet added a comment. Do you have commit access or should I commit this for you? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D92041/new/ https://reviews.llvm.org/D92041 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://list

[PATCH] D92041: [clangd] Add hover info for `this` expr

kadircet added a comment. can you give me an email address to associate the commit with? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D92041/new/ https://reviews.llvm.org/D92041 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://

[PATCH] D92370: [clang] Enable code completion of designated initializers in Compound Literal Expressions

kadircet created this revision. kadircet added reviewers: sammccall, hokein, adamcz. Herald added subscribers: cfe-commits, usaxena95. Herald added a project: clang. kadircet requested review of this revision. Herald added a subscriber: ilya-biryukov. PreferedType were not set when parsing compoun

[PATCH] D92201: [clangd] Make sure project-aware index is up-to-date for estimateMemoryUsage()

kadircet added a comment. In D92201#2421520 , @sammccall wrote: > In D92201#2419933 , @kbobyrev wrote: > >> I was trying to cause the index to load in the initialization stage for >> D92198

[PATCH] D92370: [clang] Enable code completion of designated initializers in Compound Literal Expressions

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGe98d3be11c29: [clang] Enable code completion of designated initializers in Compound Literal… (authored by kadircet). Repository: rG LLVM Github Mo

[PATCH] D92381: [clangd] Extract per-dir CDB cache to its own threadsafe class. NFC

kadircet added inline comments. Comment at: clang-tools-extra/clangd/GlobalCompilationDatabase.cpp:91 + // Whether a new CDB has been loaded but not broadcast yet. + bool Dirty = false; + // Last loaded CDB, meaningful if CachePopulated is set. maybe rename th

[PATCH] D92041: [clangd] Add hover info for `this` expr

kadircet requested changes to this revision. kadircet added a comment. This revision now requires changes to proceed. In D92041#2424928 , @njames93 wrote: > One last point, is it worth including cv qualifications in the hover info? Yes, I think we should

[PATCH] D89277: [clangd] Add $/dumpMemoryTree LSP extension

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGd0f287464d8a: [clangd] Add $/memoryUsage LSP extension (authored by kadircet). Changed prior to commit: https://reviews.llvm.org/D89277?vs=298644&

[PATCH] D89685: [clangd] Rename edge name for filesymbols to slabs in memorytree

kadircet created this revision. kadircet added a reviewer: sammccall. Herald added subscribers: cfe-commits, usaxena95, arphaman. Herald added a project: clang. kadircet requested review of this revision. Herald added subscribers: MaskRay, ilya-biryukov. This was causing duplicate `symbols` compon

[PATCH] D89297: [clangd] Add a TestWorkspace utility

kadircet added a comment. thanks this mostly looks good. can you move implementations from TestWorkspace.h to TestWorkspace.cpp ? Comment at: clang-tools-extra/clangd/unittests/FileIndexTests.cpp:431 +TEST(FileIndexTest, RelationsMultiFile) { + TestWorkspace Workspace({{"Base

[PATCH] D89685: [clangd] Rename edge name for filesymbols to slabs in memorytree

This revision was automatically updated to reflect the committed changes. Closed by commit rG40749141030b: [clangd] Rename edge name for filesymbols to slabs in memorytree (authored by kadircet). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D89685/ne

[PATCH] D89670: [clangd] Store the containing symbol for refs

kadircet added inline comments. Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:349 +SymbolRef{SM.getFileLoc(Loc), Roles, + dyn_cast_or_null(ASTNode.Parent)}); // Don't continue indexing if this is a mere reference. What

[PATCH] D89297: [clangd] Add a TestWorkspace utility

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/TestWorkspace.cpp:17 + for (const auto &Input : Inputs) { +if (!Input.second.IsMainFile) { + contin

[PATCH] D89862: [clangd] Give the server information about client's remote index protocol version

kadircet added a comment. Regarding versioning of grpc layer. In addition to including a version number in every request, looks like there's the concept of "versioned-services". So we basically change the package name to be versioned, i.e. `package clang.clangd.remote.v1` and every time we make

[PATCH] D89935: [clangd] NFC: Update FIXME comment regarding lack of c/dtor support

kadircet added a comment. Am I missing something? We still have: case index::SymbolKind::Constructor: case index::SymbolKind::Destructor: return SymbolKind::Constructor; in Protocol.cpp. E.g. Constructors and Destructors are still classified badly. I suppose the bit around `they're all

[PATCH] D89852: [clangd] Use SmallString instead of std::string in marshalling code; NFC

kadircet added a comment. Sorry I don't really follow what we are gaining by changes from string to SmallString. Could you explain if I am missing something? But I think it makes sense to get rid of Optionals in the Marshaller for `{Local,Remote}IndexRoot`, as there's no difference between `Non

[PATCH] D89935: [clangd] NFC: Update FIXME comment regarding lack of c/dtor support

kadircet added a comment. > Yeah but the LSP SymbolKind which we are converting to does not have a > destructor type, same thing with CompletionItemKind, so I guess we really do > treat ctors and dtors the same way from the LSP perspective, aren't we? Yes, and that's what the previous fixme was

[PATCH] D89935: [clangd] NFC: Update FIXME comment regarding lack of c/dtor support

kadircet added a comment. > I thought the point of the comment was us not handling it properly rather > than LSP not supporting it (e.g. LSP does support Operator but we do not). > Then, the comment about ctor and dtor being indistinguishable probably > belongs to Protocol.h/cpp and SymbolKind

[PATCH] D89852: [clangd] Get rid of llvm::Optional in Remote- and LocalIndexRoot; NFC

kadircet accepted this revision. kadircet added inline comments. This revision is now accepted and ready to land. Comment at: clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp:60 +llvm::StringRef Path(this->RemoteIndexRoot); if (!Path.endswith(PosixSepara

[PATCH] D89852: [clangd] Get rid of llvm::Optional in Remote- and LocalIndexRoot; NFC

kadircet added inline comments. Comment at: clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp:60 +llvm::StringRef Path(this->RemoteIndexRoot); +if (!is_separator(this->RemoteIndexRoot.back(), + llvm::sys::path::Style::posix)) -

[PATCH] D89852: [clangd] Get rid of llvm::Optional in Remote- and LocalIndexRoot; NFC

kadircet added inline comments. Comment at: clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp:60 +llvm::StringRef Path(this->RemoteIndexRoot); +if (!is_separator(this->RemoteIndexRoot.back(), + llvm::sys::path::Style::posix)) -

[PATCH] D90016: [clangd] NFC: Rely on ADL in llvm::sys::path calls in marshalling

kadircet added a comment. I am not really keen about trusting ADL for these, as it makes the code less explicit for reader, especially as it is not something we commonly depend on clangd so readers/reviewers would not be expecting that. So I believe we should not stray away from the general pro

[PATCH] D90016: [clangd] NFC: Rely on ADL in llvm::sys::path calls in marshalling

kadircet accepted this revision. kadircet added inline comments. This revision is now accepted and ready to land. Comment at: clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp:30 +using llvm::sys::path::append; +using llvm::sys::path::convert_to_slash; -

[PATCH] D90047: [clangd] Fix remote-server build and add it to check-clangd

kadircet created this revision. kadircet added reviewers: sammccall, kbobyrev. Herald added subscribers: cfe-commits, usaxena95, arphaman, mgorny. Herald added a project: clang. kadircet requested review of this revision. Herald added subscribers: MaskRay, ilya-biryukov. Repository: rG LLVM Gith

[PATCH] D90047: [clangd] Fix remote-server build and add it to check-clangd

This revision was automatically updated to reflect the committed changes. Closed by commit rG5dd39923a09e: [clangd] Fix remote-server build and add it to check-clangd (authored by kadircet). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D90047/new/ h

[PATCH] D90116: [clangd] Escape Unicode characters to fix Windows builds

kadircet added a comment. I think we should update cmake rules to explicitly tell the compilers that the source code language is utf8. https://docs.microsoft.com/en-us/cpp/build/reference/utf-8-set-source-and-executable-character-sets-to-utf-8?view=vs-2019 seems to be the way for MSVC. I am not

[PATCH] D90134: [clangd] Increase the TooMany limit for index-based textual navigation to 5

kadircet added a comment. > My experience using this feature in real codebases has been that we take this > early-exit too often; the limit of 3 is fairly easy to hit due to things like > layers of wrappers that repeat a function name. Bumping the limit to 5 has > increased the usefulness for m

[PATCH] D90215: [CMake] Support inter-proto dependencies in generate_protos.

kadircet added inline comments. Comment at: llvm/cmake/modules/FindGRPC.cmake:117 + # Ensure dependency headers are generated before dependent protos are built. + # FIXME: CMake docs suggest OBJECT_DEPENDS isn't needed, but I can't get the + #recommended add_dependenci

[PATCH] D90270: [clangd] Handle absolute/relative path specifications in Config

kadircet created this revision. kadircet added a reviewer: sammccall. Herald added subscribers: cfe-commits, usaxena95, arphaman. Herald added a project: clang. kadircet requested review of this revision. Herald added subscribers: ormris, MaskRay, ilya-biryukov. This introduces a mechanism for pro

[PATCH] D90270: [clangd] Handle absolute/relative path specifications in Config

kadircet updated this revision to Diff 301280. kadircet marked 9 inline comments as done. kadircet added a comment. - Drop PathSpec, deduce it from emptyness of FragmentDirectory. - Make tests more homogenous. - Add path cannonicalization to compile stage. Repository: rG LLVM Github Monorepo

[PATCH] D90270: [clangd] Handle absolute/relative path specifications in Config

kadircet added inline comments. Comment at: clang-tools-extra/clangd/ConfigProvider.cpp:124 std::vector Result; - Cache.read(FS, DC, P.FreshTime, Result); + Cache.read(FS, DC, P.FreshTime, Fragment::SourceInfo::Absolute, Result); return Result; ---

[PATCH] D90270: [clangd] Handle absolute/relative path specifications in Config

kadircet updated this revision to Diff 301311. kadircet marked 6 inline comments as done. kadircet added a comment. - Make Directory a parameter of fromYAMLFile. - Factor out configRelative logic to a helper. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm

[PATCH] D39086: Performance tracing facility for clangd.

kadircet added inline comments. Comment at: clang-tools-extra/trunk/unittests/clangd/TraceTests.cpp:57 +} +std::string VS = V->getValue(Tmp).str(); +if (VS != I->second) { RKSimon wrote: > @sammccall PVS Studio is reporting a potential null dereferenc

<    16   17   18   19   20   21   22   23   24   25   >