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

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: llvm/include/llvm/Support/FileSystem.h:1168
+/// RAII class that facilitates file locking.
+class FileLocker {
+  int FD;             ///< Locked file handle.
----------------
sepavloff wrote:
> dblaikie wrote:
> > Perhaps this type should be boolean testable (for the error), or using a factory function to create one that returns llvm::Error (wrapped around the error_code) on failure:
> > 
> > ```
> > {
> > Expected<FileLocker> F = TryLock(FD, Timeout);
> > if (!F) {
> > }
> > ...
> > }
> > ```
> > 
> > Hmm - if unlocking can fail, and that failure needs to be handled, this doesn't seem suitable for an RAII type device...
> > Perhaps this type should be boolean testable (for the error)
> 
> The class has method `locked`, which can be used to check if lock was successfully acquired, and method `error` for those applications that need more information than just the fact of failure.
> 
> > Hmm - if unlocking can fail, and that failure needs to be handled, this doesn't seem suitable for an RAII type device...
> 
> If the failure to unlock needs to be handled, the user would use method `unlock` provided by this class. In most expected use cases however this failure need not be handled. User defines RAII object and do things that require exclusive access. Upon destruction lock is released and if something goes wrong at that moment, this fact is just ignored.
>> Perhaps this type should be boolean testable (for the error)
> The class has method locked, which can be used to check if lock was successfully acquired, and method error for those applications that need more information than just the fact of failure.

Yeah, I think it'd be better if this was only accessible via a free function that returns Expected<FileLocker> so that the Error must be checked for/can't be accidentally ignored.

>> Hmm - if unlocking can fail, and that failure needs to be handled, this doesn't seem suitable for an RAII type device...
> If the failure to unlock needs to be handled, the user would use method unlock provided by this class. In most expected use cases however this failure need not be handled. User defines RAII object and do things that require exclusive access. Upon destruction lock is released and if something goes wrong at that moment, this fact is just ignored.

Fair enough/that sounds OK then. But it should return llvm::Error to make sure errors are handled/not ignored


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79066