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

[Sam McCall via Phabricator via cfe-commits]

sammccall 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>
----------------
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?


================
Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:74
+  template <typename RequestT, typename ReplyT, typename ClangdRequestT,
+            typename IndexCall, typename StreamType, typename CallbackT>
+  typename std::result_of<IndexCall(clangd::SymbolIndex &,
----------------
StreamType doesn't need to be a type param as you can make the lambda param auto


================
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
----------------
can't the return type just be `auto`?
Then I think you don't need CallbackT as a template param.


================
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) {
----------------
I don't think you need forward here... just take the param by const reference?


================
Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:42
 
+llvm::cl::opt<std::string> TracerFile(
+    "tracer-file",
----------------
kbobyrev wrote:
> kadircet wrote:
> > i think `TraceFile` is more appropriate here.
> `TracerFile` is what `ClangdMain.cpp` has; I'm not against this, but I think it'd be better to have them the same for consistency. WDYT?
(driveby, sorry)
TracerFile is the local variable name in ClangdMain, but the public interface (an environment variable there rather than a flag) is CLANGD_TRACE, and it's more important to be consistent with that.

So my vote would be --trace or --trace-file or env CLANGD_TRACE=...


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


================
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 &,
----------------
kbobyrev wrote:
> kadircet wrote:
> > 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?
> I think this is a good idea but I can't think of a nice way to do that, too. `IndexCallback` would have to update `Sent` and `FailedToSend` (`Tracer` should be outside of the callback, right?), and the callback signature if fixed so I wouldn't be able to pass these parameters by reference :( I could probably make those two fields of the class but this doesn't look very nice, or pass member function (templated one) to the callback in each index request, but this also doesn't look very nice.
> 
> I think the reason was initially to improve readability but then it's more about code deduplication: right now there are 3 pieces of code that do exactly the same, there will be a fourth one (relations request). Maybe readability even decreases but I really think that having 4 pieces of code with different types is not very nice. Also, D84525 compliments this and there will be practically only no functional code in the functions themselves.
> 
> I can understand your argument and I partially support it but I really think we're unfortunately choosing between bad and worse.
FWIW, I find the template to be very hard to read, and would prefer the version that is duplicated 4 times. 

I've left some suggestions for how to simplify (mostly, eliminating template parameters) but I'm still not sure it's worth it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84499