Eliminate all MinGW build warnings (427 → 0)#43
Conversation
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 performs a comprehensive cleanup of the codebase to satisfy the strict analysis requirements of the MinGW compiler. By addressing a wide range of warnings, the project now achieves a clean build state. In addition to the mechanical refactoring, the PR includes critical fixes for latent logic bugs that were previously hidden or ignored by the compiler. 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 performs a comprehensive refactoring to enhance type safety and code consistency across the project. Key updates include replacing NULL with appropriate literals (0 or L'\0') in non-pointer contexts, updating string comparison logic for wcscmp and strcmp, adding explicit static_cast for enum arithmetic, and fixing include typos. Feedback identifies a critical bug in memcpy calls where wide characters are copied using an incorrect byte size, suggests adopting modern C++ empty initializer lists for array zeroing, and notes a redundant line duplication in the MuHelper configuration.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request primarily focuses on codebase cleanup, including replacing NULL with 0, adding missing include guards, and applying static_cast for safer type conversions. However, the review identified three critical issues: a logic error in ZzzInventory.cpp where item IDs and model IDs are mixed, a dead code condition in the same file due to incorrect ID comparison, and a performance and memory leak issue in UIMng.cpp caused by heap allocation in the Update loop.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces extensive refactoring and bug fixes across the project, including improved type safety with explicit casts, corrected string comparison logic, and fixed array initializations. It also addresses logical errors in character actions and network protocol handling. The review feedback suggests replacing manual memory allocation in the update loop with std::vector to improve memory safety and performance.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements a broad modernization of the codebase, including the replacement of legacy NULL macros, the addition of static_cast for type safety, and the transition to std::vector for memory management. The review feedback provides actionable improvements, such as consistently using nullptr for pointer comparisons to align with C++20 standards and avoiding magic numbers in switch statements by casting original error codes. Furthermore, the reviewer correctly identified a safety risk in string formatting, suggesting the use of explicit format specifiers to handle dynamic translation strings securely.
Mosch0512
force-pushed
the
fix-build-warnings
branch
from
a54a7f6 to
310a7b0
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements widespread codebase modernization, including replacing NULL with 0 or nullptr, adding static_cast for type safety, and transitioning from raw pointer arrays to std::vector. Feedback highlights a regression in the Action() function where attack debouncing logic was removed instead of being refactored, potentially causing animation glitches. Additionally, potential out-of-bounds writes were identified in SetRivalGuildName() due to unsafe manual null-termination after wcsncpy operations.
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.
Mosch0512
force-pushed
the
fix-build-warnings
branch
from
310a7b0 to
7f5648e
Compare
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
Mechanical sweep of the i686-w64-mingw32 build warnings, plus three latent-bug fixes that the compiler's stricter analysis surfaced.
Summary
NULL → 0/L'\0',static_cast<int>on enum-arithmetic deprecated in C++20, narrowing casts inPOINT/SIZE/RECTinitializers, trailing tokens after#endif,case -1/-2:onBYTEswitches, void-pointer deletes, etc.fcaf4833): always-false&&inAction()that made abreakunreachable; deadcase 'DC'/'DT'on a single-char switch; UBdeleteof POD values held by-value in astd::map.Test plan