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

[Kadir Cetinkaya via Phabricator via cfe-commits]

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` ?)


================
Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:42
 
+llvm::cl::opt<std::string> TracerFile(
+    "tracer-file",
----------------
i think `TraceFile` is more appropriate here.


================
Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:48
+    "pretty",
+    llvm::cl::desc("Pretty-print JSON output"),
+    llvm::cl::init(true),
----------------
is it only for pretty printing trace files? do we expect to have any other components this will interact in future? (if not maybe state that in the comments)


================
Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:49
+    llvm::cl::desc("Pretty-print JSON output"),
+    llvm::cl::init(true),
+};
----------------
this sounds like a debug feature, do we really want it to be true by default?


================
Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:122
     }
-    Index->lookup(Req, [&](const clangd::Symbol &Sym) {
-      auto SerializedSymbol = ProtobufMarshaller->toProtobuf(Sym);
-      if (!SerializedSymbol)
-        return;
-      LookupReply NextMessage;
-      *NextMessage.mutable_stream_result() = *SerializedSymbol;
-      Reply->Write(NextMessage);
-    });
+    std::function<void(clang::clangd::SymbolIndex &,
+                       const clang::clangd::LookupRequest &,
----------------
all of this trickery is really nice. but i am not sure if it improves readability at all, from the perspective of call sites the amount of code is almost the same. moreover it was some trivial lambda before and instead it is lots of template parameters now.

maybe just have templated function to replace the lambda body instead? e.g.

```
template <typename StreamType, typenameResultType>
void IndexCallback(Marshaller &M, StreamType &Item) {
      trace::Span Tracer....;
      auto SerializedSymbol = M.toProtobuf(Item);
      // log the events and much more ...
}

```

then call it in the callbacks. WDYT?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84499