build wwlib on linux - #145
Conversation
|
|
||
| /* | ||
| ** Linux implementation of RegistryClass backed by an INI file. | ||
| ** Persists settings to $HOME/.config/renegade/registry.ini. | ||
| */ | ||
|
|
||
| #if !defined(_WIN32) | ||
|
|
There was a problem hiding this comment.
wwlib/openw3d.h writes its settings to openw3d.ini.
Perhaps the OpenW3D:: functions should be moved into RegistryClass and RegistryClass should write its settings to openw3d.ini in an appropriate location instead?
Also, I think we want to remove the dependency on the registry on Windows as well.
OmniBlade
left a comment
There was a problem hiding this comment.
Few things I'd personally prefer to be done in alternate ways. Not sure if others agree.
| #endif | ||
|
|
||
| #ifndef OutputDebugStringA | ||
| #define OutputDebugStringA(s) ((void)(s)) |
There was a problem hiding this comment.
I'd be more inclined to remove these and replace with either WWDEBUG_SAY or pure printf. Generally I want to remove always.h and refactor into separate headers for the things its needed for.
There was a problem hiding this comment.
reasonable to ask, but this is currently used many places in the code. I think that would make sense as a different PR?
|
|
||
| // Windows BOOL type | ||
| #ifndef BOOL | ||
| typedef int BOOL; |
There was a problem hiding this comment.
Just replace use of BOOL with int and be done with it, as winapi is removed this shouldn't be needed.
There was a problem hiding this comment.
similar to the OutputDebugString, a cursory glance indicates that there are 400+ files with a BOOL declared in them at some point, so removing it at the moment doesn't seem feasible.
|
|
||
| // Windows file operation equivalents | ||
| #ifndef DeleteFileA | ||
| #define DeleteFileA(f) (remove(f) == 0) |
There was a problem hiding this comment.
Just replace calls to DeleteFile and MoveFile with calls to remove and rename which are supported by posix and msvc.
| #endif | ||
|
|
||
| // _splitpath: split a path into drive/dir/fname/ext components | ||
| static inline void _splitpath(const char *path, char *drive, char *dir, char *fname, char *ext) |
There was a problem hiding this comment.
I'd rather this function be moved to its own header along with the constants it uses and only included where needed.
There was a problem hiding this comment.
This can probably be replaced with std::filesystem functions. It's currently used in a lot of places, so standardizing it does make sense(but probably as a separate PR)
| #if defined(_WIN32) | ||
| CRITICAL_SECTION Bar; | ||
| #else | ||
| std::mutex Bar; |
There was a problem hiding this comment.
std::mutex is supported everywhere as our base C++ version is 20, maybe just convert to use that exclusively? Also, where is the corresponding cpp implementation?
There was a problem hiding this comment.
We can probably remove this class, but this at least gets it into a state where it can compile. The cpp implementation was not changed, but I will go update that.
There was a problem hiding this comment.
actually upon further investigation the h/cpp files are not used in the build at all.
There was a problem hiding this comment.
Oh yeah, the same class with a slightly different implementation is implemented in mutex.h/cpp, can probably just remove this entirely then?
| // | ||
| ::DeleteFileA (MixFilename); | ||
| ::MoveFileA (full_path, MixFilename); | ||
| DeleteFileA (MixFilename); |
There was a problem hiding this comment.
See previous comment on dealing with these.
| /* | ||
| ** | ||
| */ | ||
| #if defined(_WIN32) |
There was a problem hiding this comment.
Probably needs a std::filesystem, SDL3 or dirent alternative implementation here though that can be TODO.
|
|
||
|
|
||
| #include "refcount.h" | ||
| #if defined(_WIN32) |
There was a problem hiding this comment.
Is the windows header needed at all if its not needed on posix?
|
|
||
| #if !defined(_WIN32) | ||
|
|
||
| #include "registry.h" |
There was a problem hiding this comment.
Ideally we want to replace the use of this class entirely throughout the codebase, so making a temp implementation seems counter productive. Maybe just stub it out instead if the intention is to just get wwlib compiling on linux to make further development easier.
| add_compile_definitions(strnicmp=strncasecmp) | ||
| add_compile_definitions(wcsnicmp=wcsncasecmp) | ||
| add_compile_definitions(wcsicmp=wcscasecmp) | ||
| add_compile_definitions(DebugBreak=__builtin_trap) |
There was a problem hiding this comment.
Not a fan of these defines being added like this, would be better to add a few headers to wrap this kind of thing instead. Like how debug break is handled here: https://github.com/TheAssemblyArmada/Thyme/blob/ccef1e11c1355c6db577a057c06e7d790f1a0333/deps/baseconfig/src/intrinsics.h#L141
There was a problem hiding this comment.
Long-term std::breakpoint would make more sense, but it's only used in 2 places at the moment so I'll simply remove it instead.
|
CODEX REVIEW Findings
Validation I fetched PR #145 locally and reviewed the full diff. |
| static void Ensure_Registry_Dir() | ||
| { | ||
| const char *home = getenv("HOME"); | ||
| if (!home) return; | ||
| char dir[512]; | ||
| snprintf(dir, sizeof(dir), "%s/.config", home); | ||
| mkdir(dir, 0755); | ||
| snprintf(dir, sizeof(dir), "%s/.config/renegade", home); | ||
| mkdir(dir, 0755); | ||
| } | ||
|
|
| static INIClass ini; | ||
| static bool loaded = false; | ||
| if (!loaded) { | ||
| loaded = true; | ||
| RawFileClass file(OpenW3D::Get_Config_File_Path()); | ||
| if (file.Is_Available()) { | ||
| ini.Load(file); | ||
| } | ||
| } | ||
| return ini; |
There was a problem hiding this comment.
This INIClass object will clash with the INIClass object created by the OpenW3D:: functions.
This sequence of actions will cause data loss:
- global
INIClassobject is initialized (throughRegistryClass), loads data from disk and stays alive - OpenW3D:: function reads ini
- OpenW3D:: function writes ini
RegistryClassmethod is called: the ini is is not read again
Build wwlib on Linux.
Mostly Claude generated, but I did clean it up slightly to better align with OpenW3D.