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

Volodymyr Sapsai via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Wed Aug 7 12:30:00 PDT 2019


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::sys::fs::remove(File->path());
+      if ((EC = llvm::sys::fs::remove(File->path())))
+        break;
 
----------------
jfb wrote:
> 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.
OK, I see now. Thanks for explanation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65545





More information about the cfe-commits mailing list