[PATCH] D102736: Fix tmp files being left on Windows builds.
Reid Kleckner via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue May 25 14:33:09 PDT 2021
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
More information about the llvm-commits
mailing list