sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.
================
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:323
+ format::DefaultFallbackStyle, Code,
+ FSProvider.getFileSystem(llvm::None).get());
if (!Style)
----------------
nit: I think we should have /*CWD=*/ on the None (and elsewhere)
================
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:402
+ InpAST->Inputs.FSProvider
+ ->getFileSystem(InpAST->Inputs.CompileCommand.Directory)
+ .get());
----------------
seems weird that we provide a working directory sometimes but not other times
when getting format style.
I think this is because not providing one is suspicious, it's not actually
needed, but sometimes we have one around?
Maybe just make getFormatStyleForFile take a FSProvider instead?
================
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:555
// different code layout.
if (auto CorrespondingFile = getCorrespondingHeaderOrSource(
+ std::string(Path), FSProvider.getFileSystem(llvm::None)))
----------------
we could plumb in threadsafefs here too...
================
Comment at: clang-tools-extra/clangd/Preamble.cpp:424
+ auto VFS = Baseline.StatCache->getConsumingFS(
+ Modified.FSProvider->getFileSystem(Modified.CompileCommand.Directory));
// First scan preprocessor directives in Baseline and Modified. These will be
----------------
I don't see a corresponding removed CD - is this a bugfix?
================
Comment at: clang-tools-extra/clangd/support/FSProvider.cpp:81
+ if (FS->setCurrentWorkingDirectory(CWD))
+ elog("Failed to set CWD in RealFileSystemProvider");
+ return FS;
----------------
Nit: RealFileSystemProvider isn't the current class.
What about `elog("VFS: failed to set CWD to {0}: {1}", CWD, Err.message())`?
================
Comment at: clang-tools-extra/clangd/support/FSProvider.h:32
virtual llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem>
- getFileSystem() const = 0;
+ getFileSystem(llvm::NoneType CWD) const = 0;
+
----------------
/// Initial working directory is arbitrary.
================
Comment at: clang-tools-extra/clangd/support/FSProvider.h:34
+
+ /// Similar to above one, except it will try to set curret working directory
+ /// to \p CWD.
----------------
"Similar to above one" seems a bit awkward, maybe "As above"?
curret -> current
================
Comment at: clang-tools-extra/clangd/support/FSProvider.h:38
+ /// StringRef conversion possible.
+ llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem>
+ getFileSystem(PathRef CWD) const;
----------------
did you want this to be virtual so that a truly virtual FS e.g. where the
working directory is just a variable can be constructed with it directly?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D81920/new/
https://reviews.llvm.org/D81920
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
