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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet 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))
----------------
hokein wrote:
> hokein wrote:
> > 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.
> >  
> I updated the patch with the `mainMessage` approach, please take a look.
i believe this still runs into the same conceptual problems. we're trying to imitate the logic done by clangdlspserver when converting a clangd-native diagnostic into an lsp-diagnostic and all of this will break as soon as there's a change in that logic. so if we want to go down that path, the best option would be to call `toLSPDiag`every time here and make sure we imitate that logic properly (e.g. right now we're relying on the ranges not being remapped because we only store fixes for the main file diagnostics).

but i feel like this is unnecessary cost both at runtime but also as code-complexity wise. this is ~the same as the data/args we publish with commands. it's just unfortunate that `data` field in `diagnostic` didn't exist pre LSP 3.16, hence we can't rely on it without breaking compatibility with some old clients (hopefully we can do that one day), hence instead of clients keeping this identifier around, I am merely suggesting to keep this alive on the server side. as conceptually this is the only layer that can do the mapping back and forth.


================
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),
----------------
hokein wrote:
> 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)
sorry i am confused. the counter has nothing to do with how we generated a tweak. it's merely counting number of times a tweak was returned. hence we don't need a new one (if a code action `Bar` was made available either through a call to `::enumerateTweaks` or `::codeAction` we should increment the same counter. clients won't be invoking both at the same time, they'll be using one or the other).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155173/new/

https://reviews.llvm.org/D155173