[PATCH] D29451: Add a prototype for clangd v0.1

[Daniel Jasper via Phabricator via cfe-commits]

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