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