[PATCH] D94503: [clangd] Explicitly avoid background-indexing the same file twice.

[Kadir Cetinkaya via Phabricator via cfe-commits]

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