djasper accepted this revision.
djasper added a comment.
Just a few nits. I think this looks like a great starting point!
================
Comment at: clangd/ClangDMain.cpp:33
+ llvm::make_unique<TextDocumentDidOpenHandler>(Outs, Logs, Store));
+ // FIXME: textDocument/didClose
+ Dispatcher.registerHandler(
----------------
Maybe add "Implement" :)
================
Comment at: clangd/ClangDMain.cpp:52
+ llvm::StringRef LineRef(Line);
+ if (LineRef.trim().empty())
+ continue;
----------------
Nit: string also has "empty()", right? Maybe only convert to StringRef after
this check.
================
Comment at: clangd/ClangDMain.cpp:62
+
+ std::cin.ignore(2); // Skip \r\n
+
----------------
Should you somehow actually read them and assert that they are "\r\n"?
================
Comment at: clangd/ClangDMain.cpp:74
+ // Log the message.
+ Logs << "<-- ";
+ Logs.write(JSON.data(), JSON.size());
----------------
So you currently always print this to std::err.. Should you guard this in DEBUG
or something?
================
Comment at: clangd/DocumentStore.h:25
+ /// Add a document to the store. Overwrites existing contents.
+ void setDocument(StringRef Uri, StringRef Text) { Docs[Uri] = Text; }
+ /// Delete a document from the store.
----------------
I'd call this addDocument for symmetry with removeDocument. The fact that you
phrase the comment that way, also make me think that this will be the more
intuitive name.
================
Comment at: clangd/JSONRPCDispatcher.cpp:89
+ Id = dyn_cast<llvm::yaml::ScalarNode>(Value);
+ } else if (KeyValue == "params") {
+ if (!Method)
----------------
I'd pull out this case and move it to the front to show that this is a
guaranteed early exit and the function can never fall through to the following
code. But I am not sure.
================
Comment at: clangd/ProtocolHandlers.h:27
+
+struct InitializeHandler : Handler {
+ InitializeHandler(llvm::raw_ostream &Outs, llvm::raw_ostream &Logs)
----------------
In the long run, we probably don't want to specify all of these in the same
file, but for now this seems fine.
================
Comment at: test/clangd/formatting.txt:9
+# CHECK: {"jsonrpc":"2.0","id":0,"result":{"capabilities":{
+# CHECK: "textDocumentSync": 1,
+# CHECK: "documentFormattingProvider": true,
----------------
How did you come up with this indentation?
https://reviews.llvm.org/D29451
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
