[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))
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155173