refactor(ini): Make the INI code cleaner and more consistent by Caball009 · Pull Request #2596 · TheSuperHackers/GeneralsGameCode

Caball009 · 2026-04-12T23:28:15Z

A handful of miscellaneous changes to the INI code. Should have zero change in behavior.

Generals and Zero Hour code are not separated, but the commit diffs are pretty clean.

greptile-apps · 2026-04-12T23:33:13Z

Greptile Summary

This PR refactors the INI class by replacing five separator/end-token member variables (m_seps, m_sepsPercent, m_sepsColon, m_sepsQuote, m_blockEndToken) with static methods, and promoting getNextToken, getNextTokenOrNull, and getNextSubToken to static. Across 23 files it also standardises loop style (while → for, token != nullptr → token) and replaces getNextTokenOrNull + explicit null-check + throw with the simpler getNextToken (which throws internally). All changes are consistent with the stated "zero behavioral change" goal — the separator strings and stricmp-based end-token comparison are identical, no callers pass nullptr explicitly to the token functions, and the %% in getSepsPercent was already present in the old initialiser.

Confidence Score: 5/5

Safe to merge; all remaining open concerns from prior threads are being addressed in separate PRs, and no new defects were introduced.

The diff is a clean mechanical refactoring with zero behavioral change confirmed: separator strings are identical, the strtok calling convention is unchanged, no callers pass explicit nullptr to the token functions (verified by search), and the getEndToken() case change is guarded by stricmp. The only previously flagged structural issue (missing = delete) is tracked in a prior thread. All remaining findings across the 23 files are P2 or below.

Core/GameEngine/Include/Common/INI.h — the missing = delete for copy/assignment is the one outstanding item, already discussed in a prior thread.

Important Files Changed

Filename	Overview
Core/GameEngine/Include/Common/INI.h	Central class interface change: 5 separator/end-token member variables removed and replaced with static methods; getNextToken/getNextTokenOrNull/getNextSubToken promoted to static; destructor declaration removed; copy-protection declarations removed without `= delete` replacement (flagged separately).
Core/GameEngine/Source/Common/INI/INI.cpp	Implementation updated to match header: member variable initializations removed, empty destructor removed, m_seps/m_blockEndToken references replaced with static calls, while→for loop conversions, token != nullptr → token simplifications, and constexpr added to local SEPS constant.
Generals/Code/GameEngine/Source/GameLogic/Object/Update/AutoDepositUpdate.cpp	getNextTokenOrNull + manual null-check + throw replaced with getNextToken (which throws internally); semantically equivalent and cleaner.
GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Update/OCLUpdate.cpp	Same getNextTokenOrNull + null-check + throw → getNextToken simplification as AutoDepositUpdate.cpp; only present in GeneralsMD version.
Generals/Code/GameEngine/Source/GameLogic/Object/ObjectCreationList.cpp	Loop variable renamed debrisName→token; double-advance pattern preserved unchanged (pre-existing behavior, separate PR planned per prior thread).

Class Diagram

%%{init: {'theme': 'neutral'}}%%
classDiagram
    class INI_Before {
        -const char* m_seps
        -const char* m_sepsPercent
        -const char* m_sepsColon
        -const char* m_sepsQuote
        -const char* m_blockEndToken
        +getSeps() const char*
        +getSepsPercent() const char*
        +getSepsColon() const char*
        +getSepsQuote() const char*
        +getNextToken(seps=nullptr) const char*
        +getNextTokenOrNull(seps=nullptr) const char*
        +getNextSubToken(expected) const char*
        ~INI()
    }
    class INI_After {
        +getSeps()$ const char*
        +getSepsPercent()$ const char*
        +getSepsColon()$ const char*
        +getSepsQuote()$ const char*
        +getEndToken()$ const char*
        +getNextToken(seps=getSeps())$ const char*
        +getNextTokenOrNull(seps=getSeps())$ const char*
        +getNextSubToken(expected)$ const char*
    }
    note for INI_After "$ = static\nMember vars removed\nDestructor is compiler-generated"

_{Reviews (3): Last reviewed commit: "Fixed forgotten change." | Re-trigger Greptile}

greptile-apps · 2026-04-12T23:33:17Z

Core/GameEngine/Include/Common/INI.h

-  INI(const INI&);
-  INI& operator=(const INI&);
-
 public:


Missing = delete for copy/assignment after removing private declarations

The PR removes the private INI(const INI&) / INI& operator=(const INI&) declarations without replacing them with = delete. m_readBuffer is heap-allocated (file->readEntireAndClose()) and released via delete[] m_readBuffer in unPrepFile(). With no copy protection, the compiler will generate a shallow-copy constructor and assignment operator, meaning two INI instances could share the same m_readBuffer pointer. When either calls unPrepFile(), the other is left with a dangling pointer — use-after-free or double-delete follows.

// Suggested fix — add in the public or class body: INI(const INI&) = delete; INI& operator=(const INI&) = delete;

Prompt To Fix With AI

This is a comment left during a code review. Path: Core/GameEngine/Include/Common/INI.h Line: 163 Comment: **Missing `= delete` for copy/assignment after removing private declarations** The PR removes the private `INI(const INI&)` / `INI& operator=(const INI&)` declarations without replacing them with `= delete`. `m_readBuffer` is heap-allocated (`file->readEntireAndClose()`) and released via `delete[] m_readBuffer` in `unPrepFile()`. With no copy protection, the compiler will generate a shallow-copy constructor and assignment operator, meaning two INI instances could share the same `m_readBuffer` pointer. When either calls `unPrepFile()`, the other is left with a dangling pointer — use-after-free or double-delete follows. ```cpp // Suggested fix — add in the public or class body: INI(const INI&) = delete; INI& operator=(const INI&) = delete; ``` How can I resolve this? If you propose a fix, please make it concise.

Generals/Code/GameEngine/Source/GameLogic/Object/ObjectCreationList.cpp

Mauller · 2026-04-13T11:03:55Z

Core/GameEngine/Include/Common/INI.h


 		this will *never* return null; if there are no more tokens, an exception will be thrown.
 	*/
-	const char* getNextToken(const char* seps = nullptr);


I wonder if we should only have one token retrieving function and let external code handle an invalid return depending on where the check is being performed, i don't like the idea of it just throwing an error due to a malformed ini, it should more gracefully handle it and warn the user.

Yeah, maybe, but would require a larger change.

I'm working on a large overhaul of the INI code, but it's a significant endeavor so I'm not sure it'll work out.

No worries, it's just nasty that the original token retrieval code throws in the first place.

Mauller · 2026-04-13T11:07:18Z

Core/GameEngine/Include/Common/INI.h

 class INI
 {
-  INI(const INI&);
-  INI& operator=(const INI&);


prob best to keep the copy and copy move constructors private.

Why is that?

the INI class is not a simple class, it has internal data handled on the heap.

The default copy constructor will only make a shallow copy of the INI, so if the original is deleted the pointed at memory in m_framebuffer will dangle to invalid memory.

the manual destructor tbh should remain but also call unprep to clear the frame buffer too.

greptile also warned about that.

Mauller · 2026-04-13T11:07:48Z

Core/GameEngine/Source/Common/INI/INI.cpp


 //-------------------------------------------------------------------------------------------------
-/*static*/ const char* INI::getNextToken(const char* seps)
+/*static*/ const char* INI::getNextToken(const char* seps /*= getSeps()*/)


No need to block comment the default argument.

xezon · 2026-04-13T20:43:47Z

GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Behavior/SlowDeathBehavior.cpp

 {
 	SlowDeathBehaviorModuleData* self = (SlowDeathBehaviorModuleData*)instance;
 	SlowDeathPhaseType sdphase = (SlowDeathPhaseType)INI::scanIndexList(ini->getNextToken(), TheSlowDeathPhaseNames);
-	for (const char* token = ini->getNextToken(); token != nullptr; token = ini->getNextTokenOrNull())


You can also leave the != nullptr.

xezon · 2026-04-13T20:47:10Z

Core/GameEngine/Source/Common/INI/INI.cpp

-	const char* SEPS = " \t,=";
-	const char *c = ini->getNextTokenOrNull(SEPS);
-	while ( c )
+	constexpr const char* SEPS = " \t,=";


Caball009 added 5 commits April 13, 2026 01:06

Changed separators from member variables to functions.

712d790

Cleaned up a few lines.

e762fbe

Replaced 'getNextTokenOrNull' + throw to 'getNextToken'.

e87afc0

Refactored while loops to for loops.

4eeb8d9

Made token naming consistent.

b6fb802

Caball009 added Minor Severity: Minor < Major < Critical < Blocker Refactor Edits the code with insignificant behavior changes, is never user facing labels Apr 12, 2026

greptile-apps bot reviewed Apr 12, 2026

View reviewed changes

Caball009 added 2 commits April 13, 2026 01:40

Made next token loops more consistent.

8bb5572

Fixed forgotten change.

61dbf3a

Caball009 mentioned this pull request Apr 13, 2026

tweak(ini): Improve consistency in use of INI::getNextTokenOrNull #2597

Draft

Mauller reviewed Apr 13, 2026

View reviewed changes

xezon reviewed Apr 13, 2026

View reviewed changes

Conversation

Caball009 commented Apr 12, 2026

Uh oh!

greptile-apps bot commented Apr 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Greptile Summary

Confidence Score: 5/5

Important Files Changed

Class Diagram

Uh oh!

greptile-apps bot Apr 12, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Mauller Apr 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

greptile-apps bot commented Apr 12, 2026 •

edited

Loading

Mauller Apr 13, 2026 •

edited

Loading