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

[Volodymyr Sapsai via Phabricator via llvm-commits]

vsapsai 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();
----------------
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?


================
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;
 
----------------
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.


================
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());
----------------
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.


================
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 + "\"");
 
----------------
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`.


================
Comment at: llvm/include/llvm/Support/FileUtilities.h:52-53
       if (DeleteIt) {
-        // Ignore problems deleting the file.
-        sys::fs::remove(Filename);
+        if (std::error_code EC = sys::fs::remove(Filename))
+          report_fatal_error("failed removing file \"" + Filename + "\": " + EC.message());
       }
----------------
For this change opinion of LLDB developers can be useful as it changes existing `FileRemover` behavior.


================
Comment at: llvm/lib/Support/GraphWriter.cpp:102
+    if (std::error_code EC = sys::fs::remove(Filename))
+      errs() << "Failed removing file\"" << Filename << "\": " << EC.message() << '\n';
     errs() << " done. \n";
----------------
Please add an extra space before `\"`


================
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;
----------------
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.


================
Comment at: llvm/lib/Support/LockFileManager.cpp:211
+      if (std::error_code EC = sys::fs::remove(UniqueLockFileName))
+        S.append("also failed to remove the lockfile");
       setError(Out.error(), S);
----------------
Do you need to start the string with a space to separate a filename and "also"?


================
Comment at: llvm/lib/Support/LockFileManager.cpp:295-297
+    report_fatal_error("failed removing file\"" + LockFileName + "\": " + EC.message());
+  if (std::error_code EC = sys::fs::remove(UniqueLockFileName))
+    report_fatal_error("failed removing file\"" + UniqueLockFileName + "\": " + EC.message());
----------------
Here too, extra space before `\"`


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65545/new/

https://reviews.llvm.org/D65545