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

[Rafael Avila de Espindola via llvm-commits]

Adrian McCarthy via Phabricator <reviews at reviews.llvm.org> writes:

> 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.

Note that this patch is *not* adding this infrastructure. It is a step
to using DeleteFile disposition, which should make it less likely that
we leave artifacts.


> ================
> 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?

The windows version uses FD only. If the passed information is not
consistent tests will fail on windows or unix.

> ================
> 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.
> ```

I will change it.


> ================
> 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/

Thanks.

>
> ================
> 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()/

I will change it.