[PATCH] D39057: [clangd][WIP] Integrate the refactoring actions into clangd

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
Comment at: clangd/ClangdServer.cpp:500
+    if (!RefactoringClient)
+      RefactoringClient = llvm::make_unique<tooling::RefactoringEditorClient>();
+    Results = clangd::findAvailableRefactoringCommands(*RefactoringClient, *AST,
----------------
Maybe initialize `RefactoringClient` in constructor?


================
Comment at: clangd/ClangdServer.cpp:514
+  std::shared_ptr<CppFile> Resources = Units.getFile(File);
+  assert(Resources && "executeCommand getCommands on non-added file");
+
----------------
s/getCommands//


================
Comment at: clangd/ClangdServer.cpp:526
+        clangd::performRefactoringCommand(*RefactoringClient, CommandName, *AST,
+
+                                          SelectionRange, Logger);
----------------
NIT: remove empty line. 


================
Comment at: clangd/Protocol.cpp:992
+                       clangd::Logger &Logger) {
+  CommandArgument Result;
+  for (auto &NextKeyValue : *Params) {
----------------
Maybe use `llvm::Optional<CommandArgument>` instead of `CommandArgument`?
If the loop body below won't get executed (i.e., `Params` is empty), we'll be returning uninitialized local variable.


================
Comment at: clangd/Protocol.h:540
+  enum Kind { SelectionRangeKind, TextDocumentIdentifierKind };
+  Kind ArgumentKind;
+
----------------
Could we make `ArgumentKind`, `union` members and default constructor private  (`makeDocumentID` , `makeSelectionRange` and `unparse` would be the only way to construct `CommandArgument`)?

This would give an interface that's much harder to misuse.


Repository:
  rL LLVM

https://reviews.llvm.org/D39057