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

[Alexandre Ganea via Phabricator via cfe-commits]

aganea added inline comments.


================
Comment at: clang/include/clang/Frontend/CompilerInstance.h:170
     std::string TempFilename;
+    Optional<llvm::sys::fs::TempFile> TempFile;
 
----------------
Can we always use `TempFile`? And remove `TempFilename` in that case?


================
Comment at: clang/lib/Frontend/CompilerInstance.cpp:728
+
+    std::string ErrorMsg;
+    if (OF.TempFile) {
----------------
I think the idea overall in LLVM is to defer the error rendering as late as possible. You could probably only keep an `Error` variable here, and render it below in the diagnostic with `toString(E)`.


================
Comment at: clang/lib/Frontend/CompilerInstance.cpp:733
+    } else {
+      if (std::error_code EC =
+              llvm::sys::fs::rename(OF.TempFilename, NewOutFile))
----------------
Here you can probably use `ECError` or `errorCodeToError()` with what is said slightly above.


================
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
----------------
Usually it is better to avoid platform-specific logic. Could we use `TempFile` all the time on all platforms?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102736