kadircet added inline comments.
================
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:748
+Context ClangdServer::createProcessingContext(PathRef File) const {
+ if (!ConfigProvider)
----------------
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.
================
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:761
+ auto DiagnosticHandler = [](const llvm::SMDiagnostic &Diag) {
+ log("config {0} at {1}:{2}:{3}: {4}",
+ Diag.getKind() == llvm::SourceMgr::DK_Error ? "error" : "warning",
----------------
kadircet wrote:
> why not elog
this is still logging though :D (not elog)
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
