[PATCH] D38570: Support: Rewrite Windows implementation of sys::fs::rename to be more POSIXy.

[Reid Kleckner via Phabricator via llvm-commits]

rnk added a subscriber: dyung.
rnk added inline comments.


================
Comment at: llvm/lib/Support/Windows/Path.inc:381
-    if (i > 0)
-      ::Sleep(1);
 
----------------
Are you sure you want to remove the Sleep? Consider the case where LLD is rapidly re-linking the same executable, and there is some silly virus scanner opening the file with a bad sharing mode. @dyung added this.


================
Comment at: llvm/lib/Support/Windows/Path.inc:400
 
-      DWORD ReplaceError = ::GetLastError();
-      ec = mapWindowsError(ReplaceError);
+  while (1) {
+    auto EC = rename_internal(FromHandle, To, true);
----------------
Let's make this fail after some time. Say 200 attempts? I doubt we need 2000... We can probably return something like `errc::resource_unavailable_try_again`` (EAGAIN)?


================
Comment at: llvm/lib/Support/Windows/Path.inc:429-430
+    // Try to find a unique new name for the destination file.
+    int UniqueId = 0;
+    while (1) {
+      std::string TmpFilename = (To + ".tmp" + utostr(UniqueId)).str();
----------------
At first, I wanted to say we should use the same random file name generation used for createUniqueEntity, but these files should not accumulate if FILE_FLAG_DELETE_ON_CLOSE works as intended.

We should limit the number of retries, though. (also 10?)


https://reviews.llvm.org/D38570