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

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev added inline comments.


================
Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:73
 private:
+  template <typename RequestT, typename ReplyT, typename ClangdRequestT,
+            typename IndexCall, typename StreamType, typename CallbackT>
----------------
sammccall wrote:
> you don't need both RequestT and ClangdRequest as template params:
>  - the things you're currently doing are all available through protobuf::Message base class
>  - but why not move the request fromProtobuf call in here so you only need to pass a single parameter?
> but why not move the request fromProtobuf call in here so you only need to pass a single parameter?
Yeah this is the idea; I have D84525 for extending `fromProtobuf` so that it can be moved here.


================
Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:75
+            typename IndexCall, typename StreamType, typename CallbackT>
+  typename std::result_of<IndexCall(clangd::SymbolIndex &,
+                                    const ClangdRequestT &, CallbackT)>::type
----------------
sammccall wrote:
> can't the return type just be `auto`?
> Then I think you don't need CallbackT as a template param.
Ah, I thought LLVM is still C++ 11, not 14. Good to know, thanks!


================
Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:96
+    } Counter(Request->DebugString());
+    return std::forward<IndexCall>(Call)(
+        *Index, ClangdRequest, [&](const StreamType &Item) {
----------------
sammccall wrote:
> kbobyrev wrote:
> > sammccall wrote:
> > > I don't think you need forward here... just take the param by const reference?
> > How do I do that? The problem is that some functions return `bool` and one `void`, so I can't really assign the result to variable and then return it.
> Currently you have 
> ```
> template <typename IndexCall>(IndexCall&& Call) {
>   forward<IndexCall>(Call);
> }
> ```
> but I think this would work fine here:
> ```
> template <typename IndexCall>(const IndexCall& Call) {
>   IndexCall(Call);
> }
> ```
> (nothing particularly wrong with this use of forward, but "more template goop" feels particularly expensive here.
Ah, you meant passing the callback by const reference. I see, thanks for the suggestion!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D84499/new/

https://reviews.llvm.org/D84499