Changeset View
Standalone View
source/lib/sysdep/os/win/wposix/wfilesystem.cpp
| /* Copyright (C) 2010 Wildfire Games. | /* Copyright (C) 2018 Wildfire Games. | ||||
| * | * | ||||
| * Permission is hereby granted, free of charge, to any person obtaining | * Permission is hereby granted, free of charge, to any person obtaining | ||||
| * a copy of this software and associated documentation files (the | * a copy of this software and associated documentation files (the | ||||
| Context not available. | |||||
| HANDLE hFile = CreateFileW(OsString(pathname).c_str(), GENERIC_WRITE, 0, 0, OPEN_EXISTING, 0, 0); | HANDLE hFile = CreateFileW(OsString(pathname).c_str(), GENERIC_WRITE, 0, 0, OPEN_EXISTING, 0, 0); | ||||
| ENSURE(hFile != INVALID_HANDLE_VALUE); | ENSURE(hFile != INVALID_HANDLE_VALUE); | ||||
| LARGE_INTEGER ofs; ofs.QuadPart = length; | LARGE_INTEGER ofs; ofs.QuadPart = length; | ||||
| WARN_IF_FALSE(SetFilePointerEx(hFile, ofs, 0, FILE_BEGIN)); | RETURN_ERROR_IF_FALSE(SetFilePointerEx(hFile, ofs, 0, FILE_BEGIN)); | ||||
| WARN_IF_FALSE(SetEndOfFile(hFile)); | RETURN_ERROR_IF_FALSE(SetEndOfFile(hFile)); | ||||
| WARN_IF_FALSE(CloseHandle(hFile)); | RETURN_ERROR_IF_FALSE(CloseHandle(hFile)); | ||||
vladislavbelov: It'd be good to have exactly code and text of the error, i.e. with `GetLastError` (https://msdn. | |||||
Not Done Inline Actionsvladislavbelov: https://msdn.microsoft.com/en-en/library/windows/desktop/ms679360(v=vs.85).aspx | |||||
Not Done Inline ActionsGetLastError returns a positive error code (1224 in this case ->"The requested operation cannot be performed on a file with a user-mapped section open.") what means that it will not be caught as an error by the caller. May I still use a macro then? Because it would be windows specific... LukeV1: `GetLastError` returns a positive error code (1224 in this case ->"The requested operation… | |||||
Not Done Inline ActionsThe whole file is Windows specific, so I think it'd be good to have a detailed error for debug. vladislavbelov: The whole file is Windows specific, so I think it'd be good to have a detailed error for debug. | |||||
Not Done Inline ActionsThe macro is called "RETURN_ERROR", but it doesn't return an error code, but it returns -1.
is what your patch uses here. If you really want to catch that error, I suppose one could introduce a new status code to namespace ERR, and then introduce a separate library filesystem function that loads the most recent error, which calls just this function on windows, and perhaps does something else sane on unix. It sounds like this could be done in a separate patch by the one who wants to work on that. I'm fine with returning FAIL = -1 until then. elexis: The macro is called "RETURN_ERROR", but it doesn't return an error code, but it returns -1.
You… | |||||
Not Done Inline ActionsI suppore the return value of wtruncate is meant to reflect the result of the ms API calls? i.e. x && y && z would return the correct non-zero result (no matter whether its signed or unsigned). On the contrary, the only call to wtruncate is RETURN_STATUS_IF_ERR(wtruncate, i.e. implying status.h values. That those MS booleans are +1 but we need FAIL = -1 is another indicator as to why it might be better to separate those two layers explicitly. So the remaining question now is whether the RETURN_STATUS_IF_ERR(wtruncate is making this confusion as well. Because it's value is int, not Status. So either it should be a non-zero or not-non-zero int, or it should be a Status type, like caller function The rest of wfilesystem.cpp returns -1 on error, which is consistent with your proposed return value. The user like file.cpp Status FileOpen do if(fd < 0) return StatusFromErrno(); // NOWARN So if you wanted to transport the error somehow, setting errno might be another consideration. I see a StatusFromWin too. And that also happens to return the thing Vladislav spoke of, GetLastError, in form of a Status! So is that RETURN_STATUS_IF_ERR(wtruncate call right? To me it looks inconsistent. That line comes from rP9350. All StatusFromWin calls are in some wposix, waio files. wopendir in this file already catches it this way. Knowingly returning a Status however also means that the return type should be Status, not int. Comparing further the functions in this file, they seem to catch the ms error only to report it instantly and then forget about it again and return -1. That would explain the return type. The other functions here return the ms return value. So I suppose we now know precisely what to do in this function? elexis: I suppore the return value of `wtruncate` is meant to reflect the result of the ms API calls? i. | |||||
| return 0; | return 0; | ||||
| } | } | ||||
| Context not available. | |||||
It'd be good to have exactly code and text of the error, i.e. with GetLastError (https://msdn.microsoft.com/ru-ru/library/windows/desktop/ms679360(v=vs.85).aspx).