[PATCH] D93393: [clangd] Ignore the static index refs from the dynamic index files.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Thanks, this should work with some slight tweaks.
Biggest question is if we really need/want to allow this function to be null.



================
Comment at: clang-tools-extra/clangd/index/Index.h:127
+  /// index or not.
+  virtual llvm::unique_function<bool(StringRef)> indexedFiles() const = 0;
+
----------------
Maybe to be really explicit, add the lifetime constraint: "The function must only be called while the index is alive."


================
Comment at: clang-tools-extra/clangd/index/Index.h:127
+  /// index or not.
+  virtual llvm::unique_function<bool(StringRef)> indexedFiles() const = 0;
+
----------------
sammccall wrote:
> Maybe to be really explicit, add the lifetime constraint: "The function must only be called while the index is alive."
it looks below like you allow this to return nullptr. TL;DR: I think we probably shouldn't, and just return false with a fixme in RemoteIndex, and false without a fixme in ProjectAwareIndex.

If we do this, we should:
 - explicitly call out that possibility, because it's error-prone
 - carefully define the semantics

In the implementations nullptr seems to be used in two ways:
 - to mean "always no", as in ProjectAwareIndex with no project.
 - to mean "always don't-know", as in RemoteIndex. The problem is 

I don't see any reason to use nullptr for "no" instead of returning `[](StringRef){return false;}`.
The "don't-know" is more appealing, because the indexes don't have to lie and instead the caller handles the ambiguity. The problem is you can't be consistent about it. Given a MergedIndex where Static->indexedFiles() is non-null and Dynamic->indexFiles() is null, then the answer can be "yes" or "don't know" depending on the file, and we can't express that, so we have to lie anyway.

We could add a tristate return value for the function, but given that it's plausible we could actually implement indexedFiles() for remote index later, I think we should just return false in the "don't know" case for now, with a FIXME there.


================
Comment at: clang-tools-extra/clangd/index/MemIndex.cpp:116
+    if (!Path)
+      return false;
+    return Files.contains(*Path);
----------------
need to `consumeError(Path.takeError())` here, or we'll crash in debug mode on error


================
Comment at: clang-tools-extra/clangd/index/Merge.cpp:127
+  return [this](llvm::StringRef FileURI) {
+    auto DynamicContainsFile = Dynamic->indexedFiles();
+    auto StaticContainsFile = Static->indexedFiles();
----------------
this is calling indexedFiles() once for each file we're checking, rather than once overall.

You want to move these variables to the capture list instead. (There should be no need to capture `this` in the end)


================
Comment at: clang-tools-extra/clangd/index/ProjectAware.cpp:121
+    return Idx->indexedFiles();
+  return nullptr;
+}
----------------
(this is definitely-no-files, so as mentioned above `[](StringRef){return false;}` seems more appropriate)


================
Comment at: clang-tools-extra/clangd/index/dex/Dex.cpp:320
+    if (!Path)
+      return false;
+    return Files.contains(*Path);
----------------
and need to consume error here


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93393