hokein added inline comments.
================
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:671
+ // FIMXE: this is tricky
+ llvm::StringRef(LSPDiag.Message)
+ .starts_with_insensitive(Diag.Message))
----------------
kadircet wrote:
> this is tricky indeed and conceptually really hard to achieve inside
> ClangdServer layer.
>
> what about keeping the cache in ClangdLSPServer, but changing the format?
> Similar to `TweakRef`, we now have a `DiagRef`, which is ClangdServer-native.
> So we can keep a cache from `(FilePath, clangd::Diagnostic)` to
> `clangd::DiagRef`, and pass those `DiagRef`s to `ClangdServer` and make sure
> we're doing the search for sure on the right domain here?
>
> this also gives us the flexibility to change the definition of a `DiagRef` in
> the future.
I'm not a fan of keeping a cache in `ClangdLSPServer`, I'd like to remove it
entirely.
What do you think about this alternative?
- pull out and exposed the `mainMessage` API from the `toLSPDiags`
- we add the `ClangdDiagnosticOptions` to the `CodeActionInputs`.
- when searching a diagnostic in `ClangdServer.cpp`, we check the equality by
checking `mainMessage(ClangdServerDiagnostic.message, Inputs.DiagOpts) ==
DiagRef.Message`
The downside here is that we have to pay `O(N * cost of mainMessage)` to find a
matched diagnostic.
================
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:683
+ // Return immediately if this is a quick-fix-only codeAction request.
+ if (Params.RequestedActionKinds.size() == 1)
+ return CB(std::move(Results));
----------------
kadircet wrote:
> we can have quick fix kind tweaks too (e.g. populate switch)
oops, I weren't aware of that, removed.
================
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:714-715
// Tracks number of times a tweak has been offered.
static constexpr trace::Metric TweakAvailable(
"tweak_available", trace::Metric::Counter, "tweak_id");
auto Action = [Sel, CB = std::move(CB), Filter = std::move(Filter),
----------------
kadircet wrote:
> can you also move this counter to global scope and use it in `::codeAction`
> too?
I added a similar local counter in `codeAction` method, since this method is
being deprecated, and removed eventually (it doesn't seem too make much sense
to use a shared counter)
================
Comment at: clang-tools-extra/clangd/ClangdServer.h:387
+ void codeAction(const CodeActionInputs &Inputs,
+ Callback<std::vector<CodeActionFix>> CB);
+
----------------
kadircet wrote:
> maybe a more structured output can be useful here:
> ```
> struct CodeActionResult {
> std::string Version; // Version of content results belong to.
> struct QuickFix {
> DiagRef D; // Diagnostic being addressed by the fix.
> Fix F;
> };
> std::vector<QuickFix> QuickFixes;
> std::vector<TweakRef> TweakRefs;
> };
> ```
good idea, also keep symmetry with the `CodeActionInputs`
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D155173/new/
https://reviews.llvm.org/D155173
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
