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

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

Remainder is just nits, looks good!



================
Comment at: clang-tools-extra/clangd/FS.cpp:126
+  // safely assume to exist is "/".
+  return "/";
+#endif
----------------
"/virtual/"


================
Comment at: clang-tools-extra/clangd/index/StdLib.cpp:79
+  // FIXME: add flags more language variants
+  if (LibraryVariant == StandardLibrarVariant::CXX14) {
+    Inputs.CompileCommand.CommandLine.push_back("-xc++-header");
----------------
nit: the unhandled case can't dynamically happen. This should probably be a switch, and the `default:` case should be llvm_unreachable("Unhandled language variant")


================
Comment at: clang-tools-extra/clangd/index/StdLib.cpp:104
+  if (!Clang) {
+    elog("tandard Library Index: Couldn't build compiler instance");
+    return nullptr;
----------------
tandard -> Standard


================
Comment at: clang-tools-extra/clangd/index/StdLib.cpp:117
+  if (!Action->BeginSourceFile(*Clang, Input)) {
+    elog("Standard Library Index:BeginSourceFile() failed");
+    return nullptr;
----------------
missing space after colon


================
Comment at: clang-tools-extra/clangd/index/StdLib.cpp:122
+  if (llvm::Error Err = Action->Execute()) {
+    elog(toString(std::move(Err)).c_str());
+    return nullptr;
----------------
elog("Standard Library Index: {0}", std::move(Err));


================
Comment at: clang-tools-extra/clangd/index/StdLib.h:34
+// 14, 17) of the standard library.
+// FIXME: add heuristic to detect this version somehow (magically).
+enum class StandardLibrarVariant { CXX14 = 0 };
----------------
The somehow/magically is by looking at the clang::LangOptions of the file being parsed :-)
This can happen if/when we move the triggering into TUScheduler which obtains and parses the command line flags.

Concretely I'd expect to resolve this FIXME by adding some function like `Optional<StandardLibraryVariant> chooseStandardLibrary(const LangOptions&)`

If this makes sense to you, you might want to make the comment a bit less hand-wavy


================
Comment at: clang-tools-extra/clangd/index/StdLib.h:35
+// FIXME: add heuristic to detect this version somehow (magically).
+enum class StandardLibrarVariant { CXX14 = 0 };
+
----------------
librarVariant ->libraryVariant


================
Comment at: clang-tools-extra/clangd/index/StdLib.h:38
+/// Generate a index of the standard library index for a given variant of
+/// the standard library. This index can be included if the translation unit
+/// does not (yet) contain any standard library headers.
----------------
I'd say rather "this index allows completion of standard library symbols whose headers have not been included yet".

The current text implies that we'd turn this off once we see `#include <vector>`, thus breaking completion of e.g `unordered_map`. I don't think we want to.


================
Comment at: clang-tools-extra/clangd/index/StdLib.h:40
+/// does not (yet) contain any standard library headers.
+std::unique_ptr<SymbolIndex> indexStandardLibrary(const ThreadsafeFS &TFS);
+
----------------
Sorry, I think I wasn't clear: the language variant should still be a parameter here, it just shouldn't have a default value. Instead the caller should pass it explicitly, this makes it obvious at the callsite that there's an imperfect assumption being made.


================
Comment at: clang-tools-extra/clangd/index/StdLib.h:45
+std::unique_ptr<SymbolIndex>
+indexUmbrellaHeaders(llvm::StringRef HeaderSources, const ThreadsafeFS &TFS,
+                     const StandardLibrarVariant &LibraryVariant);
----------------
nit: umbrellaHeader singular. (The umbrella is the file mapped to HeaderSources, the headers it includes are not umbrella headers for our purposes)


================
Comment at: clang-tools-extra/clangd/index/StdLib.h:46
+indexUmbrellaHeaders(llvm::StringRef HeaderSources, const ThreadsafeFS &TFS,
+                     const StandardLibrarVariant &LibraryVariant);
+
----------------
enums are passed by value, not const reference
(and below)


================
Comment at: clang-tools-extra/clangd/index/StdLib.h:48
+
+/// generate header containing #includes for all standard library headers
+llvm::StringRef
----------------
nit: capitalization


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105177