[PATCH] D65545: Handle some fs::remove failures
JF Bastien via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Thu Aug 1 16:08:14 PDT 2019
jfb added inline comments.
Comment at: clang/lib/Frontend/CompilerInstance.cpp:647-649
+ if (std::error_code EC = llvm::sys::fs::remove(OF.TempFilename))
+ << OF.TempFilename << EC.message();
> 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.
+ if ((EC = llvm::sys::fs::remove(File->path())))
> 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.
+ if (std::error_code EC = llvm::sys::fs::remove(IndexPath))
+ return llvm::createStringError(EC, "failed removing \"" + IndexPath + "\"");
> 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());
> 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...
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
More information about the cfe-commits