kadircet accepted this revision.
kadircet added a comment.
This revision is now accepted and ready to land.
thank, as discussed offline this looks like the right thing to do before the
branch cut. we might re-evaluate our decision around re-indexing on cmd changes
and canonicalization one day ...
LGTM! (with some small comments)
================
Comment at: clang-tools-extra/clangd/index/Background.cpp:143
Tasks.reserve(NeedsReIndexing.size());
- for (auto &Cmd : NeedsReIndexing)
- Tasks.push_back(indexFileTask(std::move(Cmd)));
+ for (const auto &Cmd : NeedsReIndexing)
+ Tasks.push_back(indexFileTask(Cmd));
----------------
nit: while here can we `s/Cmd/FilePath` ?
================
Comment at: clang-tools-extra/clangd/index/Background.cpp:144
+ for (const auto &Cmd : NeedsReIndexing)
+ Tasks.push_back(indexFileTask(Cmd));
Queue.append(std::move(Tasks));
----------------
why do we drop std::move here ? these are strings in the end and
`indexFileTask` is still expecting a value not a reference.
================
Comment at: clang-tools-extra/clangd/index/Background.cpp:360
+ if (ContextProvider) {
+ trace::Span Tracer("BackgroundPolicy");
llvm::erase_if(MainFiles, [&](const std::string &TU) {
----------------
nit: might be nice to have this in a separate patch
================
Comment at: clang-tools-extra/clangd/index/Background.h:220
enum QueuePriority {
+ ReindexFile = 0, // Implicitly applied by BackgroundQueue
IndexFile,
----------------
i suppose this is no longer needed ?
================
Comment at: clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp:868
+ Sequence.push_back(' ');
+ Q.append({A, B, A, B}); // One A is dropped, the other is zero priority.
+ } else {
----------------
both As are dropped here right ? not just one.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D94503/new/
https://reviews.llvm.org/D94503
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
