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

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/FS.h:77
 
+/// Get a virtual root node for the filesystem depending on the OS
+inline const llvm::StringLiteral virtualRoot() {
----------------
"node" doesn't mean anything here.


================
Comment at: clang-tools-extra/clangd/FS.h:79
+inline const llvm::StringLiteral virtualRoot() {
+#ifdef _WIN32
+  return "\\";
----------------
this should be defined out-of-line unless it's performance-critical for some reason.

Conditional compilation in inline bodies is a magnet for ODR violations. `_WIN32` is probably fine but no reason to scare the reader :-)


================
Comment at: clang-tools-extra/clangd/FS.h:80
+#ifdef _WIN32
+  return "\\";
+#else
----------------
This is a (drive-)relative path, we have various places we need absolute paths and may want to reuse this there. Does `C:\virtual` work?


================
Comment at: clang-tools-extra/clangd/FS.h:80
+#ifdef _WIN32
+  return "\\";
+#else
----------------
sammccall wrote:
> This is a (drive-)relative path, we have various places we need absolute paths and may want to reuse this there. Does `C:\virtual` work?
as mentioned this should ideally include some clue that it's a virtual path


================
Comment at: clang-tools-extra/clangd/index/StdLib.cpp:40
+    const ThreadsafeFS &TFS, StandardLibraryVersion LibraryVersion)
+    : VirtualUmbrellaHeaderFileName(virtualRoot().str() + "UmbrellaHeader.hpp"),
+      TFS(TFS), LibraryVersion(LibraryVersion) {}
----------------
llvm::sys::path::append


================
Comment at: clang-tools-extra/clangd/index/StdLib.cpp:40
+    const ThreadsafeFS &TFS, StandardLibraryVersion LibraryVersion)
+    : VirtualUmbrellaHeaderFileName(virtualRoot().str() + "UmbrellaHeader.hpp"),
+      TFS(TFS), LibraryVersion(LibraryVersion) {}
----------------
sammccall wrote:
> llvm::sys::path::append
don't use .hpp and rely on the driver picking the right language & version, this won't generalize. Instead, insert the flags "-xc++-header" and "-std=c++14" based on the StandardLibraryVersion


================
Comment at: clang-tools-extra/clangd/index/StdLib.cpp:48
+  // bild the string only once and cache the results in `*Once`.
+  static std::string Once = [] {
+    std::vector<llvm::StringLiteral> Headers;
----------------
the static variable must be a `string*` rather than `string` to avoid global destructors.


================
Comment at: clang-tools-extra/clangd/index/StdLib.cpp:72
+  Inputs.TFS = &TFS;
+  // TODO: can we get a real compile command from somewhere?
+  Inputs.CompileCommand.Directory = virtualRoot().str();
----------------
I'm not sure what this means, I don't think there's anything better to do here.


================
Comment at: clang-tools-extra/clangd/index/StdLib.cpp:86
+      HeaderSources, VirtualUmbrellaHeaderFileName,
+      /*RequiresNullTerminator=*/false);
+  assert(Buffer && Buffer->getBufferSize() > 0);
----------------
why false? the default (true) is what clang's parser needs


================
Comment at: clang-tools-extra/clangd/index/StdLib.cpp:97
+
+  // we only care about the symbols, so not storing the other attributes
+  auto Action = createStaticIndexingAction(
----------------
pass the default nullptr instead of a no-op lambda, it allows the indexer to skip work


================
Comment at: clang-tools-extra/clangd/index/StdLib.cpp:110
+
+  assert(IndexSlabs.Symbols && "Symbols must be set.");
+
----------------
this assertion message doesn't say anything vs the assertion itself, either drop it or say why instead


================
Comment at: clang-tools-extra/clangd/index/StdLib.h:34
+// FIXME: add feature to detect this version somehow (magically).
+enum StandardLibraryVersion { cxx14 = 0 };
+
----------------
nit: enum class to avoid polluting namespace.

llvm style capitalizes variables: `CXX14`


================
Comment at: clang-tools-extra/clangd/index/StdLib.h:34
+// FIXME: add feature to detect this version somehow (magically).
+enum StandardLibraryVersion { cxx14 = 0 };
+
----------------
sammccall wrote:
> nit: enum class to avoid polluting namespace.
> 
> llvm style capitalizes variables: `CXX14`
nit: "variant" rather than version to avoid confusion with language version?
(since this will cover c also)


================
Comment at: clang-tools-extra/clangd/index/StdLib.h:36
+
+// external interface for getting a standard library index.
+Expected<std::unique_ptr<SymbolIndex>>
----------------
The comment should be aimed at users of this module, not implementers of it :-)
This is the main API comment...


================
Comment at: clang-tools-extra/clangd/index/StdLib.h:37
+// external interface for getting a standard library index.
+Expected<std::unique_ptr<SymbolIndex>>
+indexStandardLibrary(const ThreadsafeFS &TFS,
----------------
I think I agree with your comment elsewhere that it's sufficient to return unique_ptr, indicate it might be null, and log errors.


================
Comment at: clang-tools-extra/clangd/index/StdLib.h:39
+indexStandardLibrary(const ThreadsafeFS &TFS,
+                     const StandardLibraryVersion LibraryVersion =
+                         StandardLibraryVersion::cxx14);
----------------
if introducing the enum i'd remove the default, this policy is best expressed at the call site


================
Comment at: clang-tools-extra/clangd/index/StdLib.h:42
+
+class StandardLibraryIndex {
+  // Implementation for generating the index.
----------------
This class doesn't need to be public and I don't think it needs to exist.

generateIncludeHeader is a pure function of StandardLibraryVersion, it can be a free function.

This leaves only indexHeaders, which can also be a free function of StandardLibraryVersion and TFS.
(VirtualUmbrellaHeaderFileName is purely transient state, and isn't used by any tests)


================
Comment at: clang-tools-extra/clangd/index/StdLib.h:60
+  /* generate header containing #includes for all standard library headers */
+  llvm::StringRef generateIncludeHeader();
+
----------------
as mentioned, umbrella header


================
Comment at: clang-tools-extra/clangd/index/StdLib.h:64
+  llvm::IntrusiveRefCntPtr<llvm::vfs::InMemoryFileSystem>
+  buildFilesystem(std::string HeaderSources);
+};
----------------
this is gone


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105177