kbobyrev added inline comments.
================
Comment at: clang-tools-extra/clangd/index/remote/Client.cpp:61
Callback(*Response);
+ ++Received;
}
----------------
kadircet wrote:
> shouldn't we increment this before the `continue` above ? (or rename this to
> `successful` rather than `received` ?)
Makes sense, I didn't think of "received" in terms of "overall" :P
================
Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:42
+llvm::cl::opt<std::string> TracerFile(
+ "tracer-file",
----------------
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?
================
Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:48
+ "pretty",
+ llvm::cl::desc("Pretty-print JSON output"),
+ llvm::cl::init(true),
----------------
kadircet wrote:
> 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)
Yeah, right now it's only for trace files. It's quite possible that there will
be something else that could be pretty-printed in the future, but not right
now, so I changed description (specified trace files).
================
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 &,
----------------
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.
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
