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

[JF Bastien via Phabricator via cfe-commits]

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());
+    if (std::error_code EC = llvm::sys::fs::remove(File.getKey()))
+      llvm::report_fatal_error("failed removing file \"" + File.getKey() + "\": " + EC.message());
----------------
bruno wrote:
> jfb wrote:
> > jkorous wrote:
> > > vsapsai wrote:
> > > > Clangd uses `PrecompiledPreamble` but not `TemporaryFiles` as far as I can tell. `report_fatal_error` can be really disruptive for clangd, so it would be good to get an opinion from somebody working on it if this change would impact them.
> > > Since client code in general might have different error handling/reporting strategy and can't do much about failure in destructor here, I'd consider just some kind of logging or assert here. 
> > > 
> > I changed it to `dbgs()`.
> This probably means we won't see the message in a non-debug build, right?
Yeah, so it'll be diagnosable but won't affect release builds (which I understand to be the concern expressed w.r.t. clangd).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65545