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