kadircet added a comment.
LG, mostly nits apart from a question about moving the context generation logic
into a different place.
================
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:748
+Context ClangdServer::createProcessingContext(PathRef File) const {
+ if (!ConfigProvider)
----------------
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
================
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:756
+ llvm::SmallString<256> PosixPath = File;
+ llvm::sys::path::native(PosixPath);
+ Params.Path = PosixPath.str();
----------------
also provide posix style instead of defaulted native
================
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",
----------------
why not elog
================
Comment at: clang-tools-extra/clangd/ClangdServer.h:340
+ Context createProcessingContext(PathRef) const;
+ config::Provider *ConfigProvider;
+
----------------
nit: `= nullptr;`
================
Comment at: clang-tools-extra/clangd/index/Background.h:142
+ std::function<void(BackgroundQueue::Stats)> OnProgress = nullptr,
+ std::function<Context(PathRef)> ContextProvider = nullptr);
~BackgroundIndex(); // Blocks while the current task finishes.
----------------
doesn't need to be in this patch, bu I think it is time we have an opts struct
in here.
================
Comment at: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp:879
WithContextValue CtxWithKey(TestKey, 10);
- S.run("props context", [&] {
+ S.run("props context", "somepath", [&] {
EXPECT_EQ(Context::current().getExisting(TestKey), 10);
----------------
nit: maybe extract "somepath" to a constant
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
