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

[Reid Kleckner via Phabricator via llvm-commits]

rnk added a subscriber: jansvoboda11.
rnk added a comment.

For other reviewers, I know Duncan, @dexonsmith, has also spent some time working on the clang output file management APIs. And @jansvoboda11 as well, I think.



================
Comment at: clang/lib/Frontend/CompilerInstance.cpp:720
+    // On Windows, remove deletion flags so the file can be kept and renamed.
+    llvm::sys::fs::UnmarkFileForDeletion(OF.Handle);
+
----------------
This error is unchecked, so we don't know if the disposition set call succeeds.


================
Comment at: clang/lib/Frontend/CompilerInstance.cpp:829
+        Binary ? llvm::sys::fs::OF_None : llvm::sys::fs::OF_Text;
+    // Use OF_Delete on Windows so that file can be marked for deletion.
+#ifdef _WIN32
----------------
aganea wrote:
> What do you think about having the comment inside the `#idef _WIN32` ?
Honestly, `OF_Delete` is documented as only working on Windows, so I think it would be fair to remove the ifdef, and keep this comment.


================
Comment at: clang/lib/Frontend/CompilerInstance.cpp:875
+  if (fd) {
+    llvm::sys::fs::duplicateHandle(llvm::sys::fs::convertFDToNativeFile(fd),
+                                   FileHandle);
----------------
This API also returns an error code that must be checked.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102736