[PATCH] D51641: [VFS] Cache the current working directory for the real FS.

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.


================
Comment at: lib/Basic/VirtualFileSystem.cpp:248
+private:
+  // Make sure `CWDCache` update is thread safe in `getCurrentWorkingDirectory`.
+  mutable std::mutex Mutex;
----------------
nit: it's slightly more than than, you're also making sure that concurrent getCurrentWorkingDirectory calls are coalesced.

I think that's fine and maybe not worth spelling out, but maybe the comment adds as much confusion as it resolves.


================
Comment at: lib/Basic/VirtualFileSystem.cpp:249
+  // Make sure `CWDCache` update is thread safe in `getCurrentWorkingDirectory`.
+  mutable std::mutex Mutex;
+  mutable std::string CWDCache;
----------------
CWDMutex?
If this was other state added to this class, you would *not* want it using the same mutex (because we're doing actual FS operations with the lock held)


================
Comment at: lib/Basic/VirtualFileSystem.cpp:291
   // file system abstraction that allows openat() style interactions.
-  return llvm::sys::fs::set_current_path(Path);
+  if (auto EC = llvm::sys::fs::set_current_path(Path))
+    return EC;
----------------
don't you also want to do this under the lock?

Otherwise you can write this code:

```
Notification N;
RealFileSystem FS;
async([&] { FS.setCWD("a"); FS.setCWD("b"); N.notify(); });
async([&] { N.wait(); FS.getCWD(); }
```
and the getCWD call can see "a".


Repository:
  rC Clang

https://reviews.llvm.org/D51641