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