[PATCH] D79066: [Support] Class to facilitate file locking

[Pavel Labath via Phabricator via llvm-commits]

labath added inline comments.


================
Comment at: llvm/include/llvm/Support/FileSystem.h:1179-1182
+  FileLocker(FileLocker &&L) : FD(L.FD), EC(L.EC) {
+    L.EC = make_error_code(std::errc::no_lock_available);
+  }
+
----------------
This implicitly deletes the copy constructor, but it still leaves this class with a default copy-assignment operator. Defining a move assignment would make the class safer and more consistent.


================
Comment at: llvm/include/llvm/Support/FileSystem.h:1218
+inline Expected<FileLocker>
+tryLock(int FD, std::chrono::milliseconds T = std::chrono::milliseconds(1000)) {
+  FileLocker Lock(FD, T);
----------------
I don't think there should be a default value for the timeout for two reasons:
- it makes it very unobvious that there is any timing out happening (`tryLock(FD)` looks a lot like `mutex.try_lock()` which only does an instantaneous lock check with no retries or time outs)
- the appropriate value of the timeout sounds like something that is going to be very dependent on the exact use case, and the person using it should make a deliberate choice about the value (and explain it). (Speaking of that, I am still not sure your use case really needs timeouts, though that's more of a discussion for the other patch.)


================
Comment at: llvm/include/llvm/Support/FileSystem.h:1219-1221
+  FileLocker Lock(FD, T);
+  if (Lock.locked())
+    return std::move(Lock);
----------------
If the intention is that people always use this function to lock files, then it would be better to remove the relevant FileLocker constructor and do the locking here. That would also simplify the class as it could maintain the invariant that it is created only if locking was successful and it wouldn't need to carry the error_code member.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79066/new/

https://reviews.llvm.org/D79066