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

[JF Bastien via Phabricator via cfe-commits]

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:
> > vsapsai wrote:
> > > Why are you interrupting the loop when cannot remove a file? Don't know which option is the right one, just want to know your reasons.
> > The loops already bail when `EC` is set, and here I figure if we can't remove the base file we shouldn't try to remove its corresponding timestamp.
> I was thinking about using `continue` instead of `break`. After some thinking I believe `continue` is better because this way we'll try to remove all stale module files that we can. Otherwise one bad file can prevent the module cache pruning from working.
> 
> And I agree with your logic about not removing the timestamp when cannot remove the base file.
`continue` will hit `File != FileEnd && !EC`, so it's equivalent to `break`. Your thinking makes sense, but I think we'd want to do it in a separate patch.


================
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());
----------------
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()`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65545