vsapsai added inline comments.
================
Comment at: clang/lib/Frontend/ASTUnit.cpp:2315-2316
if (Out.has_error()) {
Out.clear_error();
- return true;
+ return llvm::createStringError(Out.error(), "AST serialization failed");
}
----------------
Will `Out.error()` still work after `Out.clear_error()`?
================
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:
> > 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.
================
Comment at: llvm/include/llvm/Support/Error.h:1173
+static inline Error createStringError(std::error_code EC, const Twine &S) {
+ return createStringError(EC, S.str().c_str());
----------------
I might be missing something but should this function be `static` in the header?
================
Comment at: llvm/lib/Support/LockFileManager.cpp:58
+ if (std::error_code EC = sys::fs::remove(LockFileName))
+ report_fatal_error("Unable to remove invalid lock file \"" +
LockFileName + "\": " + EC.message());
return None;
----------------
jfb wrote:
> vsapsai wrote:
> > Do you plan to keep using `report_fatal_error` in `LockFileManager`? It
> > should help with discovering problems with modules but making it a fatal
> > error forever seems a little bit scary.
> For some of the other destructors I was thinking that failing to remove a
> file could be non-fatal, but for lock files we can't really do much if they
> stay around. I think those are probably better as fatal than any other one.
> That, or return `Expected`, but combining with
> `Optional<std::pair<std::string, int>>` is... ew...
OK, I agree that a failure to remove a lock file is bad enough to justify
`report_fatal_error`. We have some recovery code around locking (see
`unsafeRemoveLockFile`) but we can try this approach until we have evidence it
causes problems.
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
