[PATCH] D111875: [Support] [Windows] Manually clean up temp files on network shares

[Martin Storsjö via Phabricator via llvm-commits]

mstorsjo added a comment.

@rnk - Actually, we need to backtrack a bit on this. This breaks two unit tests in clang, `CrossTU/./CrossTUTests.exe/CrossTranslationUnit.CanLoadFunctionDefinition` and `CrossTU/./CrossTUTests.exe/CrossTranslationUnit.IndexFormatCanBeParsed`.

The core of the issue lies here: https://github.com/llvm/llvm-project/blob/main/clang/unittests/CrossTU/CrossTranslationUnitTest.cpp#L164-L171

The test first creates a temporary file with `llvm::sys::fs::createTemporaryFile` (which is a different thing than `TempFile` and uses `RemoveFileOnSignal` for cleanup), writes data to it, then while keeping the file open, opens with with `std::ifstream` here: https://github.com/llvm/llvm-project/blob/main/clang/lib/CrossTU/CrossTranslationUnit.cpp#L152-L157

If the temporary file is created with the `DELETE` flag, opening it a second time fails.

So with that in mind, we either need to add another open flag, `OF_DeleteAccessButDontTryToSetDeleteDisposition` which we only set in `TempFile::create`, or backtrack to just always passing `OF_Delete`, letting `setDeleteDisposition` fail silently for non-local filesystems and either call `setDeleteDisposition` another time (with a reference parameter indicating whether it succeeded or not) or just guessing with `is_local` whether it did succeed or not.

Neither of them are really clean...

Or should we unwind the whole design of using `OF_Delete` and just make `TempFile` use the same `sys::RemoveFileOnSignal` design as on unix? That would be to revert https://github.com/llvm/llvm-project/commit/3ecd20430cace7eb9e78ef0a9ff9cc95a377feb9. It doesn't say the reason for switching to this mechanism in the first place, other than "We won't see the temp file no more.". (The other major commit involved, that adjusted the design for handling `TempFile` on windows, is https://github.com/llvm/llvm-project/commit/881ba104656c40098d4bc90c52613c08136f0fe1 / D48051 <https://reviews.llvm.org/D48051>. I think parts of that can be reverted/simplified in that case then too?)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111875