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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
