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