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