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
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
