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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
