[PATCH] D65545: Handle some fs::remove failures

bruno accepted this revision. bruno added a comment. LGTM with one minor change. Comment at: clang/lib/Frontend/PrecompiledPreamble.cpp:38 +#define DEBUG_TYPE "pch" + `pch-preamble` is more accurate here. Repository: rG LLVM Github Monorepo CHANGES SINCE

[PATCH] D65545: Handle some fs::remove failures

vsapsai accepted this revision. vsapsai added a comment. This revision is now accepted and ready to land. Looks good to me. If in some situations abort on error turns out to be too aggressive, we can change it later. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://rev

[PATCH] D65545: Handle some fs::remove failures

jfb updated this revision to Diff 213980. jfb marked 3 inline comments as done. jfb added a comment. - Don't report_fatal_error in code that can be called from LLDB Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65545/new/ https://reviews.llvm.org/D

[PATCH] D65545: Handle some fs::remove failures

jfb added a comment. Updated. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65545/new/ https://reviews.llvm.org/D65545 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi

[PATCH] D65545: Handle some fs::remove failures

JDevlieghere added a comment. Thanks for looping me in, @vsapsai! Comment at: llvm/include/llvm/Support/FileUtilities.h:52-53 if (DeleteIt) { -// Ignore problems deleting the file. -sys::fs::remove(Filename); +if (std::error_code EC = sys::fs::rem

[PATCH] D65545: Handle some fs::remove failures

vsapsai added a comment. I have no other comments but for the fatal error in `FileRemover` I'd like to loop in Jonas as he was touching module cache in LLDB fairly recently. Comment at: clang/lib/Frontend/CompilerInstance.cpp:1444-1445 // Remove the file. - llvm::s

[PATCH] D65545: Handle some fs::remove failures

jfb marked 2 inline comments as done. jfb added inline comments. Comment at: clang/lib/Frontend/PrecompiledPreamble.cpp:107-111 TemporaryFiles::~TemporaryFiles() { llvm::MutexGuard Guard(Mutex); for (const auto &File : Files) -llvm::sys::fs::remove(File.getKey()); +

[PATCH] D65545: Handle some fs::remove failures

bruno added a comment. Thanks for working on this JF! Comment at: clang/lib/Frontend/PrecompiledPreamble.cpp:107-111 TemporaryFiles::~TemporaryFiles() { llvm::MutexGuard Guard(Mutex); for (const auto &File : Files) -llvm::sys::fs::remove(File.getKey()); +if (std:

[PATCH] D65545: Handle some fs::remove failures

jfb updated this revision to Diff 213497. jfb marked 8 inline comments as done. jfb added a comment. - Return llvm::Error from ASTUnit::Save - Update per comments. - Address Volodymyr's comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65545/n

[PATCH] D65545: Handle some fs::remove failures

jfb added inline comments. Comment at: clang/lib/Frontend/CompilerInstance.cpp:1444-1445 // Remove the file. - llvm::sys::fs::remove(File->path()); + if ((EC = llvm::sys::fs::remove(File->path( +break; vsapsai wrote: > jfb wrote: >

[PATCH] D65545: Handle some fs::remove failures

abrachet added a comment. > I haven't updated all ignored instances of fs::remove, I therefore can't mark > it LLVM_NODISCARD for now. I think I remember reading that casting a `[[nodiscard]]` functions return to void was broken on some compilers, ie still warns. You cast `fs::remove` to void

[PATCH] D65545: Handle some fs::remove failures

vsapsai added inline comments. Comment at: clang/lib/Frontend/ASTUnit.cpp:2315-2316 if (Out.has_error()) { Out.clear_error(); -return true; +return llvm::createStringError(Out.error(), "AST serialization failed"); } Will `Out.error()` still wor

[PATCH] D65545: Handle some fs::remove failures

jfb added inline comments. Comment at: clang/lib/Frontend/CompilerInstance.cpp:647-649 + if (std::error_code EC = llvm::sys::fs::remove(OF.TempFilename)) +getDiagnostics().Report(diag::err_fe_error_removing) + << OF.TempFilename << EC.message();

[PATCH] D65545: Handle some fs::remove failures

jkorous added inline comments. Comment at: clang/lib/Frontend/PrecompiledPreamble.cpp:107-111 TemporaryFiles::~TemporaryFiles() { llvm::MutexGuard Guard(Mutex); for (const auto &File : Files) -llvm::sys::fs::remove(File.getKey()); +if (std::error_code EC = llvm::sy

[PATCH] D65545: Handle some fs::remove failures

jfb updated this revision to Diff 212899. jfb marked 11 inline comments as done. jfb added a comment. - Return llvm::Error from ASTUnit::Save - Update per comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65545/new/ https://reviews.llvm.org/D

[PATCH] D65545: Handle some fs::remove failures

vsapsai added inline comments. Comment at: clang/lib/Frontend/CompilerInstance.cpp:647-649 + if (std::error_code EC = llvm::sys::fs::remove(OF.TempFilename)) +getDiagnostics().Report(diag::err_fe_error_removing) + << OF.TempFilename << EC.message

[PATCH] D65545: Handle some fs::remove failures

jfb created this revision. jfb added reviewers: Bigcheese, bruno, arphaman, vsapsai. Herald added subscribers: llvm-commits, cfe-commits, dexonsmith, jkorous, hiraditya. Herald added projects: clang, LLVM. We have data showing that some modules builds fail in rare cases. We're therefore interest