jfb added inline comments.
================
Comment at: clang/lib/Frontend/CompilerInstance.cpp:647-649
+ if (std::error_code EC = llvm::sys::fs::remove(OF.TempFilename))
+ getDiagnostics().Report(diag::err_fe_error_removing)
+ << OF.TempFilename << EC.message();
----------------
vsapsai wrote:
> Does the same logic as in ASTUnit.cpp apply here? I.e. if we failed to rename
> a file and emitted a message about it, should we also have a message about
> the failure to remove a file?
I've updated ASTUnit to be a bit better, and removed the logging here.
================
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:
> 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.
================
Comment at: clang/lib/Serialization/GlobalModuleIndex.cpp:935-936
// Remove the old index file. It isn't relevant any more.
- llvm::sys::fs::remove(IndexPath);
+ if (std::error_code EC = llvm::sys::fs::remove(IndexPath))
+ return llvm::createStringError(EC, "failed removing \"" + IndexPath +
"\"");
----------------
vsapsai wrote:
> Don't have a strong opinion but it looks odd that here in `createStringError`
> you are using string concatenation and a few lines lower `%s`.
Yeah this part of `createStringError` bugs me... It only takes `const char*`
for `%s`, and here I have a `Twine`. I changed it to `%s`.
================
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;
----------------
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...
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
