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

[Reid Kleckner via Phabricator via llvm-commits]

rnk added a comment.

Nice, this seems to be working out.



================
Comment at: clang/include/clang/Frontend/CompilerInstance.h:170
     std::string TempFilename;
+    Optional<llvm::sys::fs::TempFile> TempFile;
 
----------------
aganea wrote:
> Can we always use `TempFile`? And remove `TempFilename` in that case?
Having a field and type both named `TempFile` is setting off my portability alarms. I think some old versions of GCC don't like this. Maybe the field can be called `Temporary`? `TemporaryFile`? I dono.


================
Comment at: clang/lib/Frontend/CompilerInstance.cpp:708-709
+      if (OF.TempFile) {
+        if (llvm::Error E = OF.TempFile->discard())
+          consumeError(std::move(E));
       }
----------------
Can this be `consumeError(OF.TempFile->discard())`?

I find `consumeError` hard to read, I wish it were `ignoreErrors` or something like that. Short of that, please add a comment at the top of this function that we ignore any errors about failing to remove files.


================
Comment at: clang/lib/Frontend/CompilerInstance.cpp:720-721
+    if (OF.TempFile->TmpName.empty()) {
+      if (llvm::Error E = OF.TempFile->discard())
+        consumeError(std::move(E));
       continue;
----------------
Ditto re consumeError simplification


================
Comment at: clang/lib/Frontend/CompilerInstance.cpp:730
+
+    std::string ErrorMsg;
+    llvm::Error E = OF.TempFile->keep(NewOutFile);
----------------
`ErrorMsg` seems unused now


================
Comment at: clang/lib/Frontend/CompilerInstance.cpp:736
     getDiagnostics().Report(diag::err_unable_to_rename_temp)
-        << OF.TempFilename << OF.Filename << EC.message();
+        << OF.TempFile->TmpName << OF.Filename << toString(std::move(E));
 
----------------
Can you skip the toString and just do `<< std::move(E)`? I see an operator<< overload for it:
https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Basic/Diagnostic.h#L1518


================
Comment at: llvm/lib/Support/Path.cpp:1264
   FD = -1;
-
   return errorCodeToError(RenameEC);
----------------
Please revert unrelated whitespace changes


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102736