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

[Serge Pavlov via Phabricator via llvm-commits]

sepavloff marked 4 inline comments as done.
sepavloff 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);
+  }
+
----------------
labath wrote:
> 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.
Removed copy operations explicitly, added missing operator.


================
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);
----------------
labath wrote:
> 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.)
You are right, removed default argument.

Timeout is a form of protection against deadlocks. For the particular task of writing to log file infinite wait time is also acceptable, but in other use cases (such as unit tests) timeouts may be convenient.


================
Comment at: llvm/include/llvm/Support/FileSystem.h:1219-1221
+  FileLocker Lock(FD, T);
+  if (Lock.locked())
+    return std::move(Lock);
----------------
labath wrote:
> 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.
Makes sense. Moved lock logic to to function `tryLock`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79066