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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
