[PATCH] D105177: [clangd] Implemented indexing of standard library

[Christian Kühnel via Phabricator via cfe-commits]

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