[PATCH] D81998: [clangd][NFC] Rename FSProvider and getFileSystem

[Sam McCall via Phabricator via cfe-commits]

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