sammccall added a comment.
Yay for getting rid of more VFSes passed around. Not sure about some of the
other signature changes to prepareCompilerInstance. It seems a bit harder to
call now.
In particular I'm not sure whether moving the preamble-stat-cache across setup
from caller to callee is really an improvement once we modify that to wrap a
ThreadsafeFS instead of a FS.
It's just... a reminder to use the cache, I guess? But the , nullptr, nullptr
is annoying where we don't care about that.
================
Comment at: clang-tools-extra/clangd/Compiler.cpp:104
+ const PreambleFileStatusCache *FSCache) {
+ assert(FSProvider && "FSProvider is null");
assert(!CI->getPreprocessorOpts().RetainRemappedFileBuffers &&
----------------
why is this a reference if it can't be null?
(otherwise, remove the assert message - just repeats the assertion)
================
Comment at: clang-tools-extra/clangd/Compiler.h:78
std::unique_ptr<CompilerInstance> prepareCompilerInstance(
- std::unique_ptr<clang::CompilerInvocation>, const PrecompiledPreamble *,
- std::unique_ptr<llvm::MemoryBuffer> MainFile,
- IntrusiveRefCntPtr<llvm::vfs::FileSystem>, DiagnosticConsumer &);
+ std::unique_ptr<clang::CompilerInvocation> CI,
+ std::unique_ptr<llvm::MemoryBuffer> Buffer, DiagnosticConsumer
&DiagsClient,
----------------
none of the new parameter names say anything beyond repeating the types, can we
drop them?
================
Comment at: clang-tools-extra/clangd/Compiler.h:79
+ std::unique_ptr<clang::CompilerInvocation> CI,
+ std::unique_ptr<llvm::MemoryBuffer> Buffer, DiagnosticConsumer
&DiagsClient,
+ const tooling::CompileCommand &Cmd, const FileSystemProvider *FSProvider,
----------------
nit: grouping of parameters seems a bit confusing, preamble + mainfile together
(in either order) was clearer I think. DiagnosticConsumer is logically an
output param, nice to keep it at the end. Why split the FSProvider and the FS
cache, etc.
================
Comment at: clang-tools-extra/clangd/Compiler.h:80
+ std::unique_ptr<llvm::MemoryBuffer> Buffer, DiagnosticConsumer
&DiagsClient,
+ const tooling::CompileCommand &Cmd, const FileSystemProvider *FSProvider,
+ const PrecompiledPreamble *Preamble,
----------------
It doesn't seem reasonable to require both CompilerInvocation and
CompileCommand. We can pass working dir in if we must...
(CompilerInvocation actually has a working directory field, but it looks like
it potentially changes behavior. We could consider setting it in
buildCompilerInvocation() in future...)
================
Comment at: clang-tools-extra/clangd/ParsedAST.cpp:356
llvm::Optional<IncludeFixer> FixIncludes;
- auto BuildDir = VFS->getCurrentWorkingDirectory();
- if (Inputs.Opts.SuggestMissingIncludes && Inputs.Index &&
- !BuildDir.getError()) {
- auto Style = getFormatStyleForFile(Filename, Inputs.Contents, VFS.get());
+ // FIXME: Why not use CompileCommand.Directory instead?
+ if (Inputs.Opts.SuggestMissingIncludes && Inputs.Index) {
----------------
you appear to have added the fixme and... also fixed it
================
Comment at: clang-tools-extra/clangd/index/Background.cpp:273
+ /*Preamble=*/nullptr,
+ /*FSCache=*/nullptr);
if (!Clang)
----------------
hmm!
================
Comment at: clang-tools-extra/clangd/unittests/PreambleTests.cpp:78
+ Diags, PI.CompileCommand, PI.FSProvider, &BaselinePreamble->Preamble,
+ BaselinePreamble->StatCache.get());
PreprocessOnlyAction Action;
----------------
why this change?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D81720/new/
https://reviews.llvm.org/D81720
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
