[PATCH] D40328: Use a single file name to represent a lock

[Adrian McCarthy via Phabricator via llvm-commits]

amccarth added a comment.

I share zturner's concerns that file-based lock implementations on Windows are inherently fraught with pitfalls.  Can't we use a Mutex or other OS-provided synchronization mechanism?  Those will be more efficient, more robust, and they won't leave artifacts if the program is interrupted or crashes.



================
Comment at: include/llvm/Support/FileSystem.h:386
+/// FD should correspond to From. It is used instead of From when possible.
+std::error_code rename_dont_replace(const Twine &From, int FD, const Twine &To);
+
----------------
I'm not a fan of this interface, with both `From` and `FD`.  It's confusing and seems prone to being called erroneously.  What if the caller accidentally specifies a `From` that doesn't correspond to `FD`?

And it appears that neither implementation currently uses `FD`.  I see the FIXME that suggests it might be useful in the future.  Can we remove `FD` for now and revisit it later?


================
Comment at: include/llvm/Support/FileSystem.h:738
+  // already exits. This will still be a temporary file and keep or discard
+  // should still be called.
+  Error rename(const Twine &Name);
----------------
I had to read the second sentence a couple times to understand it, and, once I did, it still left me with questions.  For example, if there is an existing file with Name, does the function return an error?   May I suggest:

```
// Change the name of the temporary file.  If a file with Name already exists,
// this does nothing and returns an Error.  This will still be a temporary
// file, so keep or discard should still be called.
```


================
Comment at: lib/Support/Unix/Path.inc:433
+  // those.
+  // What we do istead is create a soft link and rename From over To. That way
+  // we avoid hard links and any reader will always see a valid file.
----------------
s/istead/instead/


================
Comment at: lib/Support/Windows/Path.inc:1042
   HANDLE H =
       ::CreateFileW(PathUTF16.begin(), Access,
                     FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE,
----------------
s/.begin()/.data()/


https://reviews.llvm.org/D40328