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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
