[PATCH] D31328: [clangd] Add code completion support

[Benjamin Kramer via Phabricator via cfe-commits]

bkramer added inline comments.


================
Comment at: clangd/ASTManager.cpp:264
+        assert(CCS->getTypedText());
+        Item.label = llvm::yaml::escape(CCS->getTypedText());
+        if (CCS->getBriefComment())
----------------
CompletionItem::unparse should do the escaping. It's weird to have YAML stuff here.


================
Comment at: clangd/ASTManager.cpp:267
+          Item.documentation = llvm::yaml::escape(CCS->getBriefComment());
+        Items.push_back(Item);
+      }
----------------
std::move


================
Comment at: clangd/ASTManager.cpp:276
+
+  std::vector<CompletionItem> getItems() const { return Items; }
+};
----------------
I think returning by reference and moving out of CodeCompletionInfo in codeComplete() is safe.


================
Comment at: clangd/ASTManager.h:78
   llvm::StringMap<std::unique_ptr<clang::ASTUnit>> ASTs;
+  std::mutex ASTLock;
+
----------------
Some documentation on the lock would be useful.

I'm a bit worried about the priorities here, code completion should always have priority over parsing for diagnostics. But we can address that later.


================
Comment at: clangd/Protocol.cpp:635
+    // flush the stream to the string.
+    Os.str();
+    // The list additionalTextEdits is guaranteed nonempty at this point.
----------------
Os.flush();


================
Comment at: clangd/Protocol.cpp:640
+  }
+  Os.str();
+  // Label is required, so Result is guaranteed to have a trailing comma.
----------------
Os.flush();


================
Comment at: clangd/Protocol.h:289
+struct CompletionItem {
+  CompletionItem()
+      : kind(CompletionItemKind::Missing),
----------------
Just use C++11 inline initialization, no need to spell out the constructor.


https://reviews.llvm.org/D31328