Do not log bogus system error in wxRenameFile() on non-Windows (PR #25985)
Lauri Nurmi
On non-Windows, there is no failed system call in the case when destination file exists, so there is no relevant system error available either.
You can view, comment on, or merge this pull request online at:
https://github.com/wxWidgets/wxWidgets/pull/25985
Commit Summary
- f153989 Do not log bogus system error in wxRenameFile() on non-Windows
File Changes
(1 file)
- M src/common/filefn.cpp (5)
Patch Links:
- https://github.com/wxWidgets/wxWidgets/pull/25985.patch
- https://github.com/wxWidgets/wxWidgets/pull/25985.diff
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#25985)Maybe I'm confused, but I think that there is no system call under Windows either: this error is only given if !overwrite but ::ReplaceFile() is called only if overwrite.
So shouldn't this wxLogSysError() be just replaced with wxLogError() unconditionally?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Lauri Nurmi
lanurmi left a comment (wxWidgets/wxWidgets#25985)Oh, apparently I was confused and misread the code. I'll push a fixup that calls wxLogError(), and additionally logs a system error if ::ReplaceFile() failed.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Lauri Nurmi
@lanurmi pushed 1 commit.
- 87af775 fixup! Do not log bogus system error in wxRenameFile() on non-Windows
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
PB
@PBfordev commented on this pull request.
> @@ -593,6 +593,14 @@ wxRenameFile(const wxString& file1, const wxString& file2, bool overwrite)
{
return true;
}
+ else
+ {
+ wxLogSysError
+ (
+ _("Failed to rename the file '%s' to '%s' by overwriting the already existing destination file"),
+ file1.c_str(), file2.c_str()
Do we need those two c_str() calls here? I thought it has been safe to pass wxString directly to wx vararg functions since at least v2.9?
Also, by changing the message, we are losing its translations...
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
@vadz commented on this pull request.
> @@ -593,6 +593,14 @@ wxRenameFile(const wxString& file1, const wxString& file2, bool overwrite)
{
return true;
}
+ else
+ {
+ wxLogSysError
+ (
+ _("Failed to rename the file '%s' to '%s' by overwriting the already existing destination file"),
+ file1.c_str(), file2.c_str()
We definitely don't need c_str()s, there are just unfortunately many of them left over.
But this is not a changed message, it's a new one, so this is not a problem.
What is a problem, however, is that we give an error before potentially succeeding in renaming the file ourselves below, which is definitely confusing and I would say wrong. I think we'll just have to live without a specific error in this case because otherwise you'd need more extensive changes (like remembering last error here and using it below, for wxMSW only).
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Lauri Nurmi
@lanurmi pushed 1 commit.
- bb56b21 Do not log bogus system error in wxRenameFile()
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.