[PATCH] D98538: [clangd] Perform merging for stale symbols in MergeIndex

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.


================
Comment at: clang-tools-extra/clangd/index/Merge.cpp:59
+
+      bool DynamicIndexIsAuthoritative =
+          // We expect the definition to see the canonical declaration, so it
----------------
this bool seems complicated enough & duplicated that you could consider pulling out

static bool indexIsAuthoritative(const SymbolIndex::IndexedFiles&, const Symbol &S);

(The IndexedFiles typedef doesn't exist, but it should!)


================
Comment at: clang-tools-extra/clangd/index/Merge.cpp:67
+      // index is stale just drop the symbol.
+      if (DynamicIndexIsAuthoritative)
         return;
----------------
we could SPAN_ATTACH a counter for this too, e.g. static_dropped

(and possibly hoist ++StaticCount to the top, static_dropped can be a subset)


================
Comment at: clang-tools-extra/clangd/unittests/IndexTests.cpp:318
+  // Even though the definition is actually deleted in the newer version of the
+  // file, we still chose to merge with information coming from static index.
   LookupRequest LookupReq;
----------------
Bah, this is actually incorrect, for the given example, right? We point to a definition that doesn't exist, in a file where the index is up to date.
Can we at least hint why we do this?
("This seems wrong, but is generic behavior we want for e.g. include headers which are always missing from the dynamic index")


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98538/new/

https://reviews.llvm.org/D98538