sammccall added inline comments.
================
Comment at: clang-tools-extra/clangd/support/FSProvider.h:1
//===--- FSProvider.h - VFS provider for ClangdServer ------------*-
C++-*-===//
//
----------------
we should rename this file as well :-(
================
Comment at: clang-tools-extra/clangd/support/FSProvider.h:24
// As FileSystem is not threadsafe, concurrent threads must each obtain one.
-class FileSystemProvider {
+class ThreadSafeFS {
public:
----------------
nit: can we have ThreadsafeFS without the extra capital S? it's about as common
(when you can't have hyphens) and easier to parse (since "safe" more clearly
binds to "thread" rather than "fs")
================
Comment at: clang-tools-extra/clangd/support/FSProvider.h:27
+ virtual ~ThreadSafeFS() = default;
/// Called by ClangdServer to obtain a vfs::FileSystem to be used for
parsing.
/// Context::current() will be the context passed to the clang entrypoint,
----------------
Now that we know what this abstraction is, the comment seems a bit out of place
layering-wise.
Maybe we should say something about context-sensitivity at the **class level**,
but probably shouldn't describe clangd's context-propagation here.
e.g.
```
Implementations may choose to depend on Context::current() e.g. to implement
snapshot semantics. clangd will not create vfs::FileSystems for use in
different contexts, so either ThreadsafeFS::view or the returned FS may contain
this logic.
```
whereas here maybe just
```
Obtain a vfs::FileSystem with an arbitrary initial working directory.
```
and below
```
Obtain a vfs::FileSystem with a specified working directory.
If the working directory can't be set (e.g. doesn't exist), logs and returns
the FS anyway.
```
================
Comment at: clang-tools-extra/clangd/support/FSProvider.h:42
-class RealFileSystemProvider : public FileSystemProvider {
+class RealFileSystemProvider : public ThreadSafeFS {
public:
----------------
RealThreadsafeFS
================
Comment at: clang-tools-extra/clangd/tool/ClangdMain.cpp:722
/* Override */ OverrideClangTidyOptions,
- FSProvider.getFileSystem(/*CWD=*/llvm::None));
+ FSProvider.view(/*CWD=*/llvm::None));
Opts.GetClangTidyOptions = [&](llvm::vfs::FileSystem &,
----------------
we need a new conventional name for these variables.
Not sure if FS works, since we may have the vfs::FileSystem in the same scope.
TFS?
Ultimately we should really get rid of any mention of FSProvider in the code, I
don't think it's a good name for anything anymore.
Not sure if it's more/less painful to do this in separate patches... the
inconsistent naming will be confusing.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D81998/new/
https://reviews.llvm.org/D81998
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
