Add non-Windows backends to a few classes#105
Conversation
caseychaos1212
left a comment
There was a problem hiding this comment.
CODEX:
Findings
High: In rawfile.cpp (line 442) the SDL3 READ|WRITE path uses SDL_IOFromFile(..., "wb+"), which truncates the file immediately. Existing callers such as tagblock.cpp (line 133) still open persistent files with Open(fname, READ|WRITE) and then read headers back, so this changes an in-place update path into silent data loss.
Medium: verchk.cpp (line 105) no longer reads the file creation time at all; it converts Get_Date_Time() instead, and the FAT day decode at verchk.cpp (line 110) is wrong ((fatDate >> 5) & 0x1f). The DATE field emitted from GameResSend.cpp (line 106) can therefore report an incorrect build timestamp.
Medium: The new SDL3 resource API tells callers to create static StaticResourceFileClass objects in rcfile.h (line 107), but those constructors register into a namespace-scope global map defined in another translation unit at rcfile.cpp (line 50). Cross-TU static initialization order is undefined, so this can fail nondeterministically before main().
| unsigned int dateTime = file->Get_Date_Time(); | ||
| unsigned int fatDate = dateTime >> 16; | ||
| unsigned int fatTime = dateTime & 0xffff; | ||
| createTime->year = 1980 + (fatDate >> 9); | ||
| createTime->month = (fatDate >> 5) & 0xf; | ||
| createTime->day = (fatDate >> 5) & 0x1f; | ||
| createTime->hour = fatTime >> 11; | ||
| createTime->minute = (fatTime >> 5) & 0x3f; | ||
| createTime->second = 2 * (fatTime & 0x1f); | ||
| return true; |
There was a problem hiding this comment.
Medium: verchk.cpp (line 105) no longer reads the file creation time at all; it converts Get_Date_Time() instead, and the FAT day decode at verchk.cpp (line 110) is wrong ((fatDate >> 5) & 0x1f). The DATE field emitted from GameResSend.cpp (line 106) can therefore report an incorrect build timestamp.
| StaticResourceFileClass(const char *filename, const void *data, size_t size) | ||
| : m_filename(filename) | ||
| , m_data(data) | ||
| , m_size(size) { | ||
| Register(); | ||
| } | ||
|
|
||
| ~StaticResourceFileClass() { | ||
| Unregister(); | ||
| } |
There was a problem hiding this comment.
Medium: The new SDL3 resource API tells callers to create static StaticResourceFileClass objects in rcfile.h (line 107), but those constructors register into a namespace-scope global map defined in another translation unit at rcfile.cpp (line 50). Cross-TU static initialization order is undefined, so this can fail nondeterministically before main().
NOTE: This class is not used by the main game, and can be removed there.
This partially reverts bab59cf9d4238a9821592e9a74dcedd1fc31a229
| NULL, OPEN_ALWAYS, FILE_ATTRIBUTE_NORMAL, NULL); | ||
| #elif defined(OPENW3D_SDL3) | ||
| Handle = fopen(Filename, "a+"); | ||
| Handle = SDL_IOFromFile(Filename, "a+"); |
There was a problem hiding this comment.
This does not preserve the Win32 OPEN_ALWAYS behavior. Per SDL’s SDL_IOFromFile docs, a+ means all writes are forced to EOF even after seeking, so this is not safe for callers that do random-access updates. For example, TagBlockFile::Save_Header() seeks back to offset 0 and rewrites the header; with append mode, that write will go to the end of the file instead of overwriting the existing header.
There was a problem hiding this comment.
Changed it to
Handle = SDL_IOFromFile(Filename, "r+");
if (Handle == nullptr) {
Handle = SDL_IOFromFile(Filename, "w+");
}Needs a review from a real human.
High: rawfile.cpp READ|WRITE path no longer truncates existing files.
- Changed SDL3 path from 'r+' fallback to 'r+b' then 'w+b', matching
OPEN_ALWAYS semantics (preserve existing file, create if absent).
Medium: verchk.cpp FAT date month field decode corrected.
- Mask was 0xf (4 bits), should be 0x1f (5 bits) for FAT month field.
Medium: rcfile.cpp static init order fix (cross-TU).
- Replaced 'extern std::unordered_map Static_Resources' with an inline
function GetStaticResources() using a function-local static (Meyers
singleton), guaranteeing construction on first use.
- cmake/embed.py generator updated.
| sources = {} | ||
| varnames = set() | ||
| for arg_src in args.sources: | ||
| match arg_src.rsplit("@", 1): |
There was a problem hiding this comment.
I get an invalid synatax error here on the version of python I happened to have locally here (Python 3.9)
| bool b_placeholder; | ||
|
|
||
| Set_Port( packet.Get(i_placeholder)); | ||
| Set_Port( packet.Get(i_placeholder)); |
There was a problem hiding this comment.
Unintended whitespace change?
| 0xBAD03605L, 0xCDD70693L, 0x54DE5729L, 0x23D967BFL, | ||
| 0xB3667A2EL, 0xC4614AB8L, 0x5D681B02L, 0x2A6F2B94L, | ||
| 0xB40BBE37L, 0xC30C8EA1L, 0x5A05DF1BL, 0x2D02EF8DL | ||
| 0x00000000U, 0x77073096U, 0xEE0E612CU, 0x990951BAU, |
There was a problem hiding this comment.
Probably don't need the U as hex is treated as unsigned by default, just an observation though.
| Handle = CreateFileA(Filename, GENERIC_READ | GENERIC_WRITE, 0, | ||
| NULL, OPEN_ALWAYS, FILE_ATTRIBUTE_NORMAL, NULL); | ||
| #elif defined(OPENW3D_SDL3) | ||
| Handle = SDL_IOFromFile(Filename, "r+"); |
There was a problem hiding this comment.
You use r+ here and w+ below. However you have rb and wb earlier in the function, don't you need b here as well to open as binary?
| return(false); | ||
| } | ||
| #elif defined(OPENW3D_SDL3) | ||
| deleteok=SDL_RemovePath(Filename)?true:false; |
There was a problem hiding this comment.
SDL_RemovePath already returns bool, isn't the ternary a bit overkill?
|
Good catch on the Python 3.9 issue in The syntax error comes from the A Python 3.9-safe equivalent is: parts = arg_src.rsplit("@", 1)
if len(parts) == 1:
p = parts[0]
path = pathlib.Path(p)
name = path.name
else:
p, name = parts
path = pathlib.Path(p)
name = name.lower()I tested that replacement locally and it works fine. |
ResourceClass: instead of using Windows-only*.resfiles, embed resources using a code generator (seecmake/embed.{cmake,py})LaunchWebBrowserfunctionmodeless dialogs were not used, so remove this abilitywdumpandW3DShellExtalso built aFileClassrelated class, I added those to the CMake project and removed some duplicated sources.