kbobyrev accepted this revision.
kbobyrev added a comment.
This revision is now accepted and ready to land.
LGTM with a couple of nits
================
Comment at: clang-tools-extra/clangd/index/dex/dexp/Dexp.cpp:85
llvm::cl::ValueDisallowed, llvm::cl::cat(llvm::cl::GeneralCategory)};
+ // FIXME: allow commands to signal failure.
virtual void run() = 0;
----------------
Nit: capitalization
================
Comment at: clang-tools-extra/clangd/index/dex/dexp/Dexp.cpp:100
+ // must do this before opts are destroyed
+ auto Cleanup = llvm::make_scope_exit(llvm::cl::ResetCommandLineParser);
if (Help.getNumOccurrences() > 0) {
----------------
nit: const?
================
Comment at: clang-tools-extra/clangd/index/dex/dexp/Dexp.cpp:328
+ if (!ExecCommand.empty())
+ return runCommand(ExecCommand, *Index) ? 0 : 1;
----------------
`return !runCommand(...)`?
================
Comment at: clang-tools-extra/clangd/index/dex/dexp/Dexp.cpp:333
+ runCommand(std::move(*Request), *Index);
return 0;
}
----------------
Maybe also just delete this one since there's some new code anyway.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D77645/new/
https://reviews.llvm.org/D77645
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
