Fix build warnings#370
Conversation
Mechanical fixes across 62 files to clear C++20-deprecation, NULL-misuse, and miscellaneous warnings flagged by the i686-w64-mingw32 GCC 13 build: - _struct.h, WSclient.h: drop trailing tokens after #endif; switch memset fill values from NULL to 0 (these two headers alone accounted for ~1500 warnings before). - 158 NULL-as-int sites converted to 0 (or L'\0' for wchar_t) at compiler- flagged lines only. - 218 deprecated enum+enum arithmetic sites wrapped with static_cast<int>(...) on the unnamed-enum operand (MODEL_TYPE_CHARM_MIXWING + EWS_*, MODEL_BODY_* + c->SkinIndex, MODEL_X - MODEL_ITEM, etc). - 9 trailing #endif tokens in GameShop/MsgBoxIGS*.cpp. - 3 narrowing conversions in POINT/SIZE/RECT brace-initializers. - 3 enum-compare sites cast to int. - 6 BYTE-typed switches with negative case literals (-1, -2) → 255, 254. - 5 delete-of-void* sites cast to the concrete pointer type before delete (xstreambuf BYTE*, CSParts CSIPartsMDL*). - 1 VLA-style `new (T[n])` → `new T[n]` (UIMng.cpp). - Stray `]` after #include in ZzzEffect.cpp. - Redundant `extern` on a definition in UIGuildInfo.cpp. - `#pragma once` removed from main-file .cpp's (NewUIGroup, NewUITextBox). - stdafx.h: NOMINMAX guarded with #ifndef so libstdc++ pre-define no longer produces a redefine warning. Six warnings remain — all flag real bugs that need human review rather than mechanical changes: TextureScript.cpp:58,63 case 'DC'/'DT' on a single-char switch (dead code) ZzzInterface.cpp:3883 'and' of mutually exclusive equal-tests is always 0 UIWindows.cpp:4215 delete on pointer to a range-for copy member (UB) Verified locally with the same toolchain CI uses (i686-w64-mingw32, Ninja, Release, BUILD_TESTING=OFF). Build links cleanly; final warning count: 6.
Each was a real bug rather than a style nit; mechanical "silence the warning" fixes weren't appropriate. ZzzInterface.cpp:3311-3312 (-Wlogical-op-parentheses-equivalent: 'and' of mutually exclusive equal-tests is always 0) The block at line 3308 filters by `CurrentAction` range while excluding specific values via `!=`. The two final clauses used `==` against PLAYER_FENRIR_SKILL_ONE_RIGHT and PLAYER_RAGE_FENRIR_ONE_RIGHT, making the whole condition unsatisfiable (CurrentAction can't equal two values at once) and the guarded `break` permanently dead. Surrounding pattern shows the intent was to *exclude* those Fenrir actions from the range check, so the operators are flipped to `!=`. TextureScript.cpp:58-66 (-Wmultichar + -Wswitch-outside-range) `switch (strTokenFile[i])` is over a single `char`; cases `'DC'` and `'DT'` are multi-character constants that can never match a `char` value. Pure dead code — removed the two `case` blocks; no behavior change. UIWindows.cpp:4204-4218 (-Wfree-nonheap-object) CLetterList::ClearLetterTextCache iterated the m_LetterCache map by value copy and called `delete &cachedLetter.second` on a stack-local POD copy — undefined behavior. RemoveLetterTextCache had the same UB pattern via an iterator into the map's internal storage; the compiler missed it because the iterator's lifetime is fuzzier, but it's equally invalid. FS_LETTER_TEXT is a POD with no owning members, and m_LetterCache stores values directly (std::map<DWORD, FS_LETTER_TEXT>), so destruction is handled correctly by std::map::erase / std::map::clear. Both `delete` calls removed (the local `letter` variables become unused and are dropped). Final warning count: 0.
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on resolving various build warnings that have accumulated in the project. The changes involve type safety improvements, such as explicit casting for enum arithmetic, and code modernization, including the replacement of null pointer constants and the use of safer memory management practices. These adjustments improve code stability and ensure compliance with modern C++ standards. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements widespread code cleanup and modernization, focusing on type safety with static_cast, correcting string comparison logic, and replacing NULL with 0 or nullptr. It also improves memory management by using std::vector and cleans up preprocessor directives. Feedback identifies a high-severity bug in WSclient.cpp where wchar_t buffers are incorrectly handled with byte-based memset and memcpy, potentially leading to undefined behavior. Additionally, a regression was noted regarding the removal of action-checking logic which disables attack debouncing; the reviewer recommends implementing the suggested include list of animations to properly maintain functionality.
The loop walked GuildMark[] looking for the rival guild's index, but
built its comparison key by copy/casting raw wire bytes:
wchar_t Temp[8 + 1];
memset(Temp, 0, 8); // 8 bytes = 4 wchar_t (of 9)
memcpy(Temp, (wchar_t*)Data->Name, 8); // UTF-8 bytes punned as wide
Temp[8] = 0;
if (wcscmp(p->GuildName, Temp) == 0)
Two bugs stacked: Data->Name is char[8] of UTF-8 (every other call site
goes through CMultiLanguage::ConvertFromUtf8 - see line 7445), so the
wchar_t* cast pairs adjacent UTF-8 bytes into wide-char gibberish; and
the byte counts on memset/memcpy are wchar-count-times-2 short of the
buffer's 18 bytes, leaving slots 4-7 uninitialized.
ConvertFromUtf8 already populates GuildWarName from Data->Name 19 lines
above, and nothing mutates GuildWarName before the loop. Drop the broken
local Temp rebuild and compare against GuildWarName directly.
Effect: GuildWarIndex now actually gets set when the war target matches
a player's guild-mark slot. Previously this loop's matches were
non-deterministic at best; the renderer was relying on the working
FindGuildName(GuildWarName) path at line 426 to set GuildWarIndex.
The wchar_t-string variant of the texture-script parser had zero callers across the codebase (grep -rn '\bparsingTScript\b' returned only the definition and the header declaration). Only parsingTScriptA(char*) is ever invoked - by ZzzBMD.cpp:2834 (BMD load) and SMD2BMD.cpp:235 (model converter), both of which pass char-string texture names from BMD blobs. The deleted wchar_t function also had a latent fall-through bug that would never have surfaced in production because it was never called: its `case L'D':` branch handled L'C' / L'T' lookahead correctly, but the loop's next iteration saw the consumed character (L'C' or L'T'), hit the `default:` arm, set m_bBeScript = false, and returned. Anyone reviving this would need to advance `i` inside the D case in addition to wiring up callers. Removes 73 lines of code that compiled but couldn't run, ran but couldn't return success, and could-have-returned but no asset asks for it.
1 participant
Handling the build warnings from our action workflows :D