[PATCH] D102736: Fix tmp files being left on Windows builds.

[Adrian McCarthy via Phabricator via llvm-commits]

amccarth added a comment.

Cool!  This is getting much simpler.  My remaining comments are mostly musings and you can tell me to buzz off.

But I'd like to see @aganea's questions addressed.  It does seem redundant to have to keep the file name when we already have an object that represents the file.



================
Comment at: llvm/lib/Support/Path.cpp:1237
       RenameEC = copy_file(TmpName, Name);
       setDeleteDisposition(H, true);
     }
----------------
I'm curious if this path has ever been exercised.

I see that rename_handle is implemented with MoveFileExW on Windows.  MoveFileExW docs say it will fail if you're trying to move a _directory_ to another device, but that it can do copy+delete to move a _file_ to another device.  (That might require adding MOVEFILE_COPY_ALLOWED to the flags passed in the MoveFileExW in lib\Support\Windows\Path.inc near line 485.)

Anyway, it just seems like we're re-implementing functionality the OS calls can already do for us.

https://docs.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-movefileexw


================
Comment at: llvm/lib/Support/Path.cpp:1243
   if (RenameEC)
     setDeleteDisposition(H, true);
 #else
----------------
If I understand correctly, we're using the delete disposition flag to get the OS to clean up the file in case we crash.  But does that prevent us from just deleting it outright once we know it's safe to do so?

In other words if we could just delete the file here, then the divergence between Win32 and others could possibly be reduced.


================
Comment at: llvm/lib/Support/Path.cpp:1253
   }
   sys::DontRemoveFileOnSignal(TmpName);
 #endif
----------------
Possibly stupid idea:  What if RemoveFileOnSignal and DontRemoveFileOnSignal did the Win32-specific delete disposition flag twiddling?  With that encapsulated, and assuming we can still explicitly delete a Windows file that's already marked for deletion, it seems like all of the special handling here could go away.

Then again, the ...OnSignal things are in sys rather than sys::fs, so maybe it's not such a good idea.

I'm not trying to make more work for Amy.  The immediate goal probably isn't well served by this level of overthinking.  But it would be nice to see all this get simpler in the long term.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102736