sammccall marked 2 inline comments as done.
sammccall added inline comments.
================
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:748
+Context ClangdServer::createProcessingContext(PathRef File) const {
+ if (!ConfigProvider)
----------------
kadircet wrote:
> sammccall wrote:
> > kadircet wrote:
> > > why not make this a free function in `ConfigProvider.h` ?
> > >
> > > that way we could just keep passing ConfigProvider around rather than a
> > > derived lambda.
> > > It makes testing more cumbersome, but enables us to move the logic out of
> > > ClangdServer
> > Oh, I thought hiding this logic in ClangdServer was a feature!
> >
> > For instance, if we want to report configuration errors as LSP diagnostics
> > or as notifications, we need to have access to the ClangdServer to do that.
> hmm, I thought we would rather make DiagnosticHandler an input parameter in
> such a scenario, (we even have the alias `config::DiagnosticCallback` 😅) with
> a nullptr just logging the errors and warnings (as in current implementation).
>
> I don't see any other interaction but surfacing diagnostics, and I believe it
> would look nicer if we provided a callback for diags when creating context
> with configs. But this one also gets the job done, so up to you.
If i'm understanding what you mean (pass a callback in clangdserver::options?)
I don't think it's the right level of abstraction. E.g. for diagnostics, we do
have a lifecycle and data model around diagnostics that clangdserver is
responsible for producing, and delivering raw diagnostics one at a time doesn't
seem enough.
Similarly if we want warning/error notifications that seems like a higher level
thing to add to clangdserver::callbacks.
(If it's the ClangdServer builder's responsibility to handle the errors, then
the ConfigProvider interface isn't quite the right one in ClangdServer::Options
- why expose the errors to clangdserver if they're just going to be passed back
again?)
Anyway, we probably can't know for sure until we come to do richer error
reporting, which isn't a top priority at the moment. This is my best guess for
where we should handle errors (or at least translate them) but I don't think
it's terribly hard to move later. So I'll leave as-is for now, to unblock the
more critical parts of this patch.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D83095/new/
https://reviews.llvm.org/D83095
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
