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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
