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