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

[Amy Huang via Phabricator via cfe-commits]

akhuang marked 2 inline comments as done.
akhuang added a comment.

In D102736#2767432 <https://reviews.llvm.org/D102736#2767432>, @aganea wrote:

> Do you think the existing crash tests can be modified to validate that .tmp files are deleted indeed?

Looks like the existing crash tests use `#pragma clang __debug crash` but in the case where clang crashes it's actually fine. I guess the only cases where this patch actually applies is when the program is killed -- don't know if there's a good way to do that in the test suite?



================
Comment at: clang/lib/Frontend/CompilerInstance.cpp:733
+    } else {
+      if (std::error_code EC =
+              llvm::sys::fs::rename(OF.TempFilename, NewOutFile))
----------------
aganea wrote:
> Here you can probably use `ECError` or `errorCodeToError()` with what is said slightly above.
I think this path goes away if we always use TempFile


================
Comment at: clang/lib/Frontend/CompilerInstance.cpp:840
     int fd;
-    std::error_code EC = llvm::sys::fs::createUniqueFile(
-        TempPath, fd, TempPath,
-        Binary ? llvm::sys::fs::OF_None : llvm::sys::fs::OF_Text);
+#if _WIN32
+    // On Windows, use llvm::sys::fs::TempFile to write the output file so
----------------
aganea wrote:
> Usually it is better to avoid platform-specific logic. Could we use `TempFile` all the time on all platforms?
Hm, probably, that would make things simpler. 


================
Comment at: llvm/lib/Support/Path.cpp:1253
   }
   sys::DontRemoveFileOnSignal(TmpName);
 #endif
----------------
amccarth wrote:
> 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.
@rnk suggested that too - I think it would definitely make things nicer, but yeah, the fact that it's in sys and not sys::fs is inconvenient.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102736