sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.
Both side-effects seem fine to me.
> EnableBackgroundIndex option controls whether the component will be created
> at all
Do you think we should eventually switch to always setting
ClangdServer::Options::BackgroundIndex to true in ClangdServerMain (or removing
the option?)
If not, I'm not sure if there's much benefit in also setting the config flag...
> CompileCommandsDir is also used by clangd --check workflow, which doesn't use
> configs
We can/should fix this though, right?
================
Comment at: clang-tools-extra/clangd/tool/ClangdMain.cpp:555
+ };
+ if (Sync) {
+ IndexLoadTask();
----------------
(This seems a little weird, get passed in a task runner and maybe ignore it.
Seems like the asyncprojectindex should know whether it's supposed to be sync
or not, and either pass in a null runner or it should handle the asynchrony
itself... not something for this patch)
================
Comment at: clang-tools-extra/clangd/tool/ClangdMain.cpp:566
+
+class CommandLineFlagsConfigProvider : public config::Provider {
+ std::vector<config::CompiledFragment>
----------------
or just FlagsConfigProvider?
================
Comment at: clang-tools-extra/clangd/tool/ClangdMain.cpp:570
+ config::DiagnosticCallback) const override {
+ config::CompiledFragment Frag = [](const config::Params &, Config &C) {
+ if (!CompileCommandsDir.empty()) {
----------------
Hmm, it seems natural to move assembling some of these objects into the
constructor, I'm not sure it matters much, at least for now, but putting the
actual flag-inspection on the hot-ish path seems constraining.
(Lowest-ceremony I can come up with is just making them e.g.
Optional<ExternalIndexSpec> members of the provider, and then having a single
fragment which capture the provider `this` and keep the conditionals inside it)
================
Comment at: clang-tools-extra/clangd/tool/ClangdMain.cpp:756
+ CompileCommandsDir = Path.str().str();
+ // FIXME: Still set for clangd --check. Use config in --check workflow
+ // and get rid of these options?
----------------
If we fix this, we can move the rest of the logic into the flags config
provider constructor.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D98029/new/
https://reviews.llvm.org/D98029
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
