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

[Kadir Cetinkaya via Phabricator via cfe-commits]

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, [&](const auto &Item) {
+      auto SerializedItem = ProtobufMarshaller->toProtobuf(Item);
----------------
usage of `auto` in here results in a templated-lambda. I don't think we want that (please let me know if I am missing something) maybe go back to using clangd::Symbol ? (same for other lambdas)


================
Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:203
+      TracerStream.reset();
+      llvm::errs() << "Error while opening trace file " << TraceFile << ": "
+                   << EC.message();
----------------
any reasons for not using elog in here (and in other places using `llvm::errs()` ?


================
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 &,
----------------
sammccall wrote:
> 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.
> 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.

right, i've missed that bit. but I was suggesting you to call `IndexCallback` inside the lambda, e.g. callsites would look like this:

```
    Index->lookup(Req, [&](const auto &Item) {
        IndexCallback(*ProtobufMarshaller, Item);
    });
```

so in theory you could pass in `Sent` and `FailedToSend` as parameters to `IndexCallback` and mutate them in there.


but not really relevant anymore.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84499