[PATCH] D65545: Handle some fs::remove failures

[JF Bastien via Phabricator via llvm-commits]

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