ioeric accepted this revision.
ioeric added a comment.
This revision is now accepted and ready to land.
lgtm
================
Comment at: clangd/ClangdLSPServer.cpp:148
+ if (Trace)
+ (*Trace)["Reply"] = *Result;
+ Server.reply(ID, json::Value(std::move(*Result)));
----------------
do we really want to stash all responses in trace? It sounds like it could get
pretty spammy (e.g. with completion results).
================
Comment at: clangd/ClangdLSPServer.cpp:184
+ mutable std::mutex RequestCancelersMutex;
+ llvm::StringMap<std::pair<Canceler, unsigned>> RequestCancelers;
+ unsigned NextRequestCookie = 0;
----------------
nit: while we are here, could you add some documentation for these two fields?
================
Comment at: clangd/ClangdLSPServer.cpp:409
+
+ Reply(std::move(*Items));
+ },
----------------
We are not translating internal errors from ClangdServer to LSP errors anymore.
It seems fine and makes code cleaner. Just want to ask if this is intentional.
================
Comment at: clangd/ClangdLSPServer.h:34
+/// The server also supports $/cancelRequest (MessageHandler provides this).
+class ClangdLSPServer : private DiagnosticsConsumer {
public:
----------------
Just out of curiosity, why does `DiagnosticsConsumer` implemented in
ClangdLSPServer instead of ClangdServer?
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D53387
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
