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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
