[PATCH] D148783: [clangd] Add support TextDocumentEdit.

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

> can you also have a version of the clang-tools-extra/clangd/test/fixits-command.test with documentChanges support? it's unlikely to have clients in that configuration but i believe the deserialization issue i mentioned above would be discoverable by such a test.

I'm happy to add a test for that, but I'm not sure the deserialization issue you mentioned in the comment, is the one to use `mapOptional`?



================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.h:201
   void bindMethods(LSPBinder &, const ClientCapabilities &Caps);
-  std::vector<Fix> getFixes(StringRef File, const clangd::Diagnostic &D);
+  std::pair<std::optional<int64_t>, std::vector<Fix>>
+  getFixes(StringRef File, const clangd::Diagnostic &D);
----------------
kadircet wrote:
> instead of a pair maybe a:
> ```
> struct VersionedFixes {
>   std::optional<int64_t> DocumentVersion;
>   std::vector<Fix> Fixes;
> };
> ```
we don't need this struct now, as we switch to store the `CodeAction`.


================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.h:236
   std::mutex FixItsMutex;
   typedef std::map<clangd::Diagnostic, std::vector<Fix>, LSPDiagnosticCompare>
       DiagnosticToReplacementMap;
----------------
kadircet wrote:
> can we instead have a:
> ```
> std::map<clangd::Diagnostic, std::vector<CodeAction>, LSPDiagnosticCompare> Fixes;
> ```
> 
> We'll make sure we store code actions with necessary version information.
> That way `FixItsMap` can stay the same, and rest of the code will look more uniform; we'll do the conversion from Fixes to CodeActions during `onDiagnosticsReady`
good idea! 


================
Comment at: clang-tools-extra/clangd/Protocol.cpp:735
   llvm::json::ObjectMapper O(Params, P);
-  return O && O.map("changes", R.changes);
+  return O && O.map("changes", R.changes) && O.map("documentChanges", R.documentChanges);
 }
----------------
kadircet wrote:
> we actually want `O.mapOptional` for both "changes" and "documentChanges".
I think `map` is a better fit here, it has a specific version of `std::optional`, see https://github.com/llvm/llvm-project/blob/main/llvm/include/llvm/Support/JSON.h#L852.

`mapOptional`  doesn't set the field when missing the key in json object,


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148783/new/

https://reviews.llvm.org/D148783