[PATCH] D78896: [Support] Add file lock/unlock functions

[Pavel Labath via Phabricator via llvm-commits]

labath added inline comments.


================
Comment at: llvm/include/llvm/Support/FileSystem.h:1140-1141
+/// @param File    The descriptor representing the file to lock.
+/// @param Timeout Time in milliseconds that the process should wait before
+///                reporting lock failure.
+/// @returns errc::success if lock is successfully obtained,
----------------
this would be nicer as a std::chrono type.


================
Comment at: llvm/include/llvm/Support/FileSystem.h:1145
+/// error_code otherwise.
+std::error_code lockFile(int FD, unsigned Timeout = 0);
+
----------------
Should this be called `tryLockFile`, or maybe even `tryLockFileFor` (to mirror e.g. [[ https://en.cppreference.com/w/cpp/thread/timed_mutex/try_lock_for | std::timed_mutex::try_lock_for ]]) ?


================
Comment at: llvm/unittests/Support/Path.cpp:1938-1939
+#ifdef _WIN32
+// Windows refuses lock request if file region is already locked by the same
+// process. POSIX system in this case updates the existing lock.
+TEST_F(FileSystemTest, lockFile) {
----------------
Have you considered using flock(2) instead of F_SETLK? That might give you semantics which are a bit saner and a bit closer to what happens on windows (though file locking is always weird on posix systems)...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78896