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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

In D148783#4286652 <https://reviews.llvm.org/D148783#4286652>, @hokein wrote:

>> 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`?

yes it was for `mapOptional`, which turns out not to be an issue as there's a specialization for `optional<T>`.

but still having that test to verify deserialization logic wouldn't hurt.



================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:1746
+
+                 if (DiagOpts.EmbedFixesInDiagnostics && !CodeActions.empty()) {
+                   Diag.codeActions.emplace(CodeActions);
----------------
we didn't have the empty check before, let's not introduce it now (i.e. we'll still reply with an empty set of code actions rather than "none" when there are no fixes known to clangd)


================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:1751-1753
                  auto &FixItsForDiagnostic = LocalFixIts[Diag];
-                 llvm::copy(Fixes, std::back_inserter(FixItsForDiagnostic));
+                 llvm::move(std::move(CodeActions),
+                            std::back_inserter(FixItsForDiagnostic));
----------------
i am not sure why this logic was appending to the vector rather than just overwriting. but we shouldn't receive the same diagnostics from the callback here, am i missing something? so what about just:
```
LocalFixIts[Diag] = std::move(CodeActions);
```


================
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);
 }
----------------
hokein wrote:
> 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,
yeah you're right, i missed the specialization of `map` for `optional<T>`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148783