[PATCH] D155173: [clangd] Refine the workflow for diagnostic Fixits.

[Haojian Wu via Phabricator via cfe-commits]

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