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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
