[PATCH] D115232: [clangd] Indexing of standard library

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

thanks LG, mostly nits and a couple of questions



================
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:91
+    if (Tasks)
+      Tasks->runAsync("IndexStdlib", std::move(Task));
+    else
----------------
I suppose this should be rare hence won't bite us in practice, but might worth having a comment mentioning this creates tasks with no barriers.


================
Comment at: clang-tools-extra/clangd/index/StdLib.cpp:44
+  }
+  return L == CXX ? llvm::StringLiteral("vector") : "stdio.h";
+}
----------------
`llvm_uncreachable` instead?


================
Comment at: clang-tools-extra/clangd/index/StdLib.cpp:49
+  if (LO.CPlusPlus) {
+    return !LO.CPlusPlus11   ? LangStandard::lang_cxx98
+           : !LO.CPlusPlus14 ? LangStandard::lang_cxx11
----------------
nit: this feels a little bit hard to read, what about:
```
if(2b) return 2b;
if(20) return 20;
...
return 98;
```


================
Comment at: clang-tools-extra/clangd/index/StdLib.cpp:56
+  }
+  return !LO.C11 ? LangStandard::lang_c99
+         // C17 has no new features, so treat C14 as C17.
----------------
same here


================
Comment at: clang-tools-extra/clangd/index/StdLib.cpp:92
+  // The umbrella header is the same for all versions of each language.
+  // Headers that are unsupported in old lang versions are usually guarded by
+  // #if. Some headers may be not present in old stdlib versions, the umbrella
----------------
maybe move second half of this comment into `buildUmbrella` ?


================
Comment at: clang-tools-extra/clangd/index/StdLib.cpp:121
+// We want to drop these, they're a bit tricky to identify:
+//  - we don't want to limit to symbols our our list, as our list has only
+//    top-level symbols (and there may be legitimate stdlib extensions).
----------------
s/our our/to our/


================
Comment at: clang-tools-extra/clangd/index/StdLib.cpp:131
+// another header that defines a symbol from our stdlib list.
+static SymbolSlab filter(SymbolSlab Slab, const StdLibLocation &Loc) {
+  SymbolSlab::Builder Result;
----------------
drop static, and move into previous anonymous namespace ?


================
Comment at: clang-tools-extra/clangd/index/StdLib.cpp:142
+         })
+      Set->insert(Name);
+    return Set;
----------------
s/Name/Header/ ?


================
Comment at: clang-tools-extra/clangd/index/StdLib.cpp:160
+    if (!S.IncludeHeaders.empty() &&
+        StandardHeaders.contains(S.IncludeHeaders.front().IncludeHeader)) {
+      GoodHeader[S.CanonicalDeclaration.FileURI] = true;
----------------
`StandardHeaders` are always in verbatim format, but are we sure if that holds for the `IncludeHeader` ?

I suppose that should be the case since we map known path suffixes back to a verbatim umbrella header, I just wonder how exhaustive the list is. (it might be nice to manually check no implementation detail headers falls out of cracks)


================
Comment at: clang-tools-extra/clangd/index/StdLib.cpp:177
+  }
+  for (const auto &Good : GoodHeader)
+    if (Good.second)
----------------
seems like debugging artifact, either drop or put in `#ifndef NDEBUG`


================
Comment at: clang-tools-extra/clangd/index/StdLib.cpp:204
+      !CI->getPreprocessorOpts().ImplicitPCHInclude.empty()) {
+    elog("Indexing standard library failed: bad CompilerInvocation");
+    assert(false && "indexing stdlib with a dubious CompilerInvocation!");
----------------
why log if we think we can't hit this case?


================
Comment at: clang-tools-extra/clangd/index/StdLib.cpp:216
+  IgnoreDiagnostics IgnoreDiags;
+  CI->getPreprocessorOpts().clearRemappedFiles();
+  auto Clang = prepareCompilerInstance(
----------------
why are we doing this exactly? once we override the same file multiple times, I believe the last one takes precedence. it's definitely safer to clear the remapped files here rather than relying on that fact, but I am afraid of losing other mappings, possibly set outside of clangd's knowledge (e.g. through flags like `-remap-file`?)


================
Comment at: clang-tools-extra/clangd/index/StdLib.cpp:233
+  // Sadly we can't use IndexOpts.FileFilter to restrict indexing scope.
+  // Files from outside the location may define true std symbols anyway.
+  // We end up "blessing" such headers, and can only do that by indexing
----------------
what does `the location` refer to here? I think we should also stress the fact that even when indexing the same file, we have a good chance of seeing different symbols due to PP directives (and different std versions)


================
Comment at: clang-tools-extra/clangd/index/StdLib.cpp:237
+
+  // Refs, relations, containers in the stdlib mostly aren't useful.
+  auto Action = createStaticIndexingAction(
----------------
s/containers/include graph


================
Comment at: clang-tools-extra/clangd/index/StdLib.cpp:256
+  if (HadErrors) {
+    log("Errors when generating the standard library index, index may be "
+        "incomplete");
----------------
i'd make this part of the next log


================
Comment at: clang-tools-extra/clangd/index/StdLib.cpp:299
+
+  if (!Config::current().Index.StandardLibrary) {
+    dlog("No: disabled in config");
----------------
maybe move this check to the top


================
Comment at: clang-tools-extra/clangd/index/StdLib.cpp:313
+    llvm::StringRef DirPath = llvm::sys::path::parent_path(HeaderPath);
+    if (!HS.getFileMgr().getVirtualFileSystem().getRealPath(DirPath, Path))
+      SearchPaths.emplace_back(Path);
----------------
why do we resolve the symlinks ?


================
Comment at: clang-tools-extra/clangd/index/StdLib.cpp:323
+      llvm::vfs::Status Stat;
+      if (!HS.getFileMgr().getNoncachedStatValue(Path, Stat) &&
+          Stat.isRegularFile())
----------------
any reason for going with `Noncached` version? (clangd doesn't set one up today, but not relying on that would be nice if we don't have a particular concern here)


================
Comment at: clang-tools-extra/clangd/index/StdLib.cpp:329
+    case DirectoryLookup::LT_Framework:
+      // stdlib can't be a framework (framework includes bust have a slash)
+      continue;
----------------
s/bust/must


================
Comment at: clang-tools-extra/clangd/index/StdLib.h:66
+  // This function is threadsafe.
+  llvm::Optional<StdLibLocation> add(const LangOptions &, const HeaderSearch &);
+
----------------
maybe drop the optinal and bail out in indexing when `Paths` are empty ?


================
Comment at: clang-tools-extra/clangd/index/StdLib.h:68
+
+  // Indicates whether we a built index should be used.
+  // It should not be used if a newer version has subsequently been added.
----------------
s/a built/built an/


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115232