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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
