kuhnel added a comment.
> Main points in the implementation are:
>
> - simplify the exposed interface
Good point, I added a new function `indexStandardLibrary()` as external
interface.
> - i don't think we need to mess with VFS at all actually
Yes, removed that.
> - we should think a little about which index data we actually want to keep/use
I removed Refs, Relations and Graph as per your comment. However I have to
admit, I don't know what they are and how they are used.
What's a good place to look at so that I learn what they do?
> Next design questions seem to be about lifetime/triggering:
>
> - how many configurations of stdlib index to have
> - when do we build the indexes, and who owns them
> - what logic governs whether/which stdlib index is triggered, and where do we
> put it
While you're out, I'll try to set up something so that I can do some manual
tests with a real standard library.
================
Comment at: clang-tools-extra/clangd/index/StdLib.cpp:98
+ auto Buffer = FS->getBufferForFile(StdLibHeaderFileName);
+ if (!Buffer)
+ return error("Could not read file from InMemoryFileSystem");
----------------
sammccall wrote:
> CreateMemBuffer instead to avoid expressing this impossible error condition?
Not sure what you mean with CreateMemBuffer.
I replaced the `if` with an `assert` as this should not fail.
================
Comment at: clang-tools-extra/clangd/index/StdLib.cpp:101
+ auto Clang = prepareCompilerInstance(std::move(CI), /*Preamble=*/nullptr,
+ std::move(*Buffer), FS, IgnoreDiags);
+ if (!Clang)
----------------
sammccall wrote:
> hang on, if we're providing the overridden buffer to prepareCompilerInstance
> (like we do for edited files in clangd that may or may not be saved on disk
> yet) then why do we need the VFS at all?
We don't need the VFS at all, you're right. Handing over a buffer is good
enough.
================
Comment at: clang-tools-extra/clangd/unittests/StdLibIndexTests.cpp:47
+ // available on the machine
+ std::string HeaderMock = R"CPP(
+ int myfunc(int a);
----------------
sammccall wrote:
> I'm not sure we're testing what we want to be testing here.
>
> In real life, the symbols are not going to be in the entrypoint, but a file
> included from it.
> And clangd's indexer *does* treat the main file differently from others.
>
> It's pretty awkward to actually fix but maybe worth a comment.
So you would prefer that we change `HeaderMock` to only contain `#include` and
then create the mock headers as virtual files in the MockFS?
================
Comment at: clang-tools-extra/clangd/unittests/StdLibIndexTests.cpp:56
+ Req.Query = "myfunc";
+ Req.AnyScope = true;
+ Req.Limit = 100;
----------------
sammccall wrote:
> AnyScope and Limit are not needed
`AnyScope` is actually needed, otherwise the result is empty.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D105177/new/
https://reviews.llvm.org/D105177
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
