kadircet 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.
================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:1722
[&](clangd::Diagnostic Diag, llvm::ArrayRef<Fix> Fixes) {
+ if (DiagOpts.EmbedFixesInDiagnostics && !Fixes.empty()) {
+ Diag.codeActions.emplace();
----------------
hokein wrote:
> This part of code is moved from the `toLSPDiags`, we can keep it unchanged
> then we have to pass the `Version` and `SupportsDocumentChanges` to
> `toLSPDiags` which makes the API ugly. Open for ideas.
no i think it makes more sense in LSP layer, as it's an LSP extension. I
believe we had it in the guts because i believe we were using it internally
with a client that doesn't use LSPServer at some point.
it would be better to drop the extension but i see that qt-creator &
sourcekit-lsp has mentions of this :/ so as I mentioned above, we can convert
the fixes to CodeActions here unconditionally (it's not more expensive than the
copy we perform today). afterwards we only make copying those CodeActions into
the Diag conditioned on the capability.
================
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);
----------------
instead of a pair maybe a:
```
struct VersionedFixes {
std::optional<int64_t> DocumentVersion;
std::vector<Fix> Fixes;
};
```
================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.h:236
std::mutex FixItsMutex;
typedef std::map<clangd::Diagnostic, std::vector<Fix>, LSPDiagnosticCompare>
DiagnosticToReplacementMap;
----------------
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`
================
Comment at: clang-tools-extra/clangd/Diagnostics.cpp:426
-CodeAction toCodeAction(const Fix &F, const URIForFile &File) {
+CodeAction toCodeAction(const Fix &F, const URIForFile &File,
+ const std::optional<int64_t> &Version,
----------------
we can now move this into clangdlsperver.cpp
================
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);
}
----------------
we actually want `O.mapOptional` for both "changes" and "documentChanges".
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D148783/new/
https://reviews.llvm.org/D148783
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
