refactor(messaging): replace GameMessage argument linked list with std::vector by RikuAnt · Pull Request #2700 · TheSuperHackers/GeneralsGameCode

RikuAnt · 2026-05-11T22:03:25Z

GameMessage and NetGameCommandMsg stored their arguments in a linked list (m_argList/m_argTail/m_argCount/m_next). Since arguments are only ever appended and iterated in order, this is replaced with a std::vector, removing the manual pointer threading and simplifying all related code. A DEBUG_ASSERTCRASH is added to guard the existing 255-argument limit.

AI usage was minimal. All code has been reviewed and tested to best of my capability.

TODO:

Replicate in Generals.

…rs#2630) Simpler and cleaner control. Improves loop performance significantly via cache hits and brings random index access down to O(1)

Only affects parts that relied on the linked list functionality of the GameMessageArguments

…uperHackers#2630)

greptile-apps · 2026-05-11T22:11:25Z

Greptile Summary

Replaces the manual linked-list argument storage (m_argList/m_argTail/m_argCount/m_next) in GameMessage and NetGameCommandMsg with std::vector<const GameMessageArgument*>, eliminating manual pointer threading throughout construction, destruction, and iteration. All callers (Recorder, NetGameCommandMsg copy constructor, constructGameMessage) are updated consistently and deep-copy semantics are preserved.

GameMessageArgument::m_next is removed; allocArg() and both destructors are simplified to a plain range-for with const_cast on delete — correct because all objects are originally allocated as mutable.
getArgumentCount() is now inlined in the header with a DEBUG_ASSERTCRASH to guard the existing 255-argument contract, and getArguments() exposes a const vector reference for bulk iteration.
getArgument(i) and getArgumentDataType(i) use an unsigned-cast bounds check that correctly handles out-of-range and negative indices without changing observable behaviour.

Confidence Score: 5/5

Safe to merge — the refactor is mechanical and consistent across all five files with no logic changes to argument handling.

Every code path that previously walked the linked list now iterates the vector with equivalent semantics. Memory ownership is correctly transferred (mutable allocation, const storage, const_cast on delete). The two vector-by-value copies are minor efficiency nits with no correctness impact.

No files require special attention.

Important Files Changed

Filename	Overview
GeneralsMD/Code/GameEngine/Include/Common/MessageStream.h	Replaces m_argList/m_argTail/m_argCount linked-list fields with std::vector<const GameMessageArgument*>; adds getArguments() accessor and moves getArgumentCount() inline with a DEBUG_ASSERTCRASH guard for the 255-arg limit. Clean — no UB, const-correctness is consistent.
GeneralsMD/Code/GameEngine/Source/Common/MessageStream.cpp	allocArg(), destructor, getArgument(), and getArgumentDataType() all updated to use the new vector; negative-index safety preserved via unsigned-cast trick in bounds checks; const_cast in destructor is correct since allocArg always allocates mutable objects.
Core/GameEngine/Include/GameNetwork/NetCommandMsg.h	m_numArgs/m_argSize/m_argList/m_argTail replaced with a single std::vector<const GameMessageArgument*> m_argList; forward declaration of GameMessageArgument added. Change is self-contained.
Core/GameEngine/Source/GameNetwork/NetCommandMsg.cpp	Constructor, copy-from-GameMessage constructor, destructor, addArgument, and constructGameMessage all updated; deep-copy semantics in copy constructor are correct. One unnecessary vector copy (const by value instead of const ref) noted.
GeneralsMD/Code/GameEngine/Source/Common/Recorder.cpp	Loop updated to iterate via getArguments(); same unnecessary vector copy as NetCommandMsg.cpp constructor — should bind by const reference.

Sequence Diagram

sequenceDiagram
    participant GMsg as GameMessage
    participant ArgVec as m_argList (vector)
    participant Net as NetGameCommandMsg
    participant Rec as RecorderClass

    GMsg->>ArgVec: allocArg() push_back(newArg)
    Net->>GMsg: getArguments() returns const ref
    Net->>Net: deep copy via addArgument()
    Net->>ArgVec: push_back(newArg)
    Rec->>GMsg: getArguments() returns const ref
    Rec->>Rec: writeArgument per element
    GMsg->>ArgVec: destructor deleteInstance each element
    Net->>ArgVec: destructor deleteInstance each element

Prompt To Fix All With AI

Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 2
GeneralsMD/Code/GameEngine/Source/Common/Recorder.cpp:746
`getArguments()` returns a `const` reference, but the variable is declared without `&`, so the vector is copied on every call. This triggers a heap allocation proportional to the argument count. Using a `const` reference avoids the copy entirely.

```suggestion
	const std::vector<const GameMessageArgument*>& args = msg->getArguments();
```

### Issue 2 of 2
Core/GameEngine/Source/GameNetwork/NetCommandMsg.cpp:103
`getArguments()` returns a `const` reference, but `args` is declared by value, causing an unnecessary vector copy. Binding to a `const` reference avoids the extra heap allocation while keeping the same semantics since no modification is made to the vector.

```suggestion
	const std::vector<const GameMessageArgument*>& args = msg->getArguments();
```

_{Reviews (3): Last reviewed commit: "refactor: refactor GameMessageArguments ..." | Re-trigger Greptile}

Caball009 · 2026-05-11T22:26:23Z

+	// TheSuperHackers @refactor RiQQ 11/05/2026 Replaced linked list with std::vector for argument storage
 	GameMessage::Type m_type;
-	GameMessageArgument *m_argList, *m_argTail;
+	std::vector<GameMessageArgument*> m_argList;


Can this be std::vector<const GameMessageArgument*>?

This collides with the pooled object delete functionality deleteInstance in NetGameCommandMsg destructor. ~~We can use const_cast if this is the preferred way?~~ I made the change in favor of const_cast over the reinterpret_cast that the AI first recommended

I see. The const_cast is not ideal, but that's price we've to pay, I suppose. I think the current way with <const T*> is preferable, but I'll let this conversation stay unresolved for now.

(const_cast is right here; I'm not sure whether reinterpret_cast would even compile here).

…ersions (TheSuperHackers#2630)

… unnecessary test before uint conversion (TheSuperHackers#2630)

- Return GameMessageArguments as const - Style fixes (remove blank space, indent and space out too packed methods) - Remove vector includes; already included from precompiled header

…et_cast (TheSuperHackers#2630)

Caball009 · 2026-05-13T00:54:00Z

-	for (Int i = 0; i < count; ++i) {
-		addArgument(msg->getArgumentDataType(i), *(msg->getArgument(i)));
+
+	const std::vector<const GameMessageArgument*> args = msg->getArguments();


const std::vector<const GameMessageArgument*>& args, we don't need to make a copy.

Caball009 · 2026-05-13T00:55:42Z

+
+	const std::vector<const GameMessageArgument*> args = msg->getArguments();
+	m_argList.reserve(args.size());
+	for (size_t i = 0; i < args.size(); ++i) {


nit: Could use an empty line between reserve and the loop.

Caball009 · 2026-05-13T00:59:24Z

-//			lasttype = type;
-//		}
-		writeArgument(msg->getArgumentDataType(i), *(msg->getArgument(i)));
+	const std::vector<const GameMessageArgument*> args = msg->getArguments();


Also const T& to avoid a copy.

Caball009 · 2026-05-13T01:14:36Z

+	// TheSuperHackers @refactor RiQQ 11/05/2026 Replaced linked list with std::vector for argument storage
 	GameMessage::Type m_type;
-	GameMessageArgument *m_argList, *m_argTail;
+	std::vector<GameMessageArgument*> m_argList;


I see. The const_cast is not ideal, but that's price we've to pay, I suppose. I think the current way with <const T*> is preferable, but I'll let this conversation stay unresolved for now.

(const_cast is right here; I'm not sure whether reinterpret_cast would even compile here).

Caball009 · 2026-05-13T01:15:44Z

 #include "GameNetwork/NetPacketStructs.h"
 #include "Common/UnicodeString.h"
-
+class GameMessageArgument;


nit: Could use an empty line between includes and forward declarations.

Caball009 · 2026-05-13T01:28:22Z

Good effort.

Generals fails to compile for now, but it's probably most convenient to wait for xezon's feedback before you replicate the changes from ZH to Gen.

RikuAnt added 3 commits May 12, 2026 00:04

perf: refactor message argument handling to use vector (TheSuperHacke…

0bf542d

…rs#2630) Simpler and cleaner control. Improves loop performance significantly via cache hits and brings random index access down to O(1)

perf: adjust network messaging to use vectors (TheSuperHackers#2630)

572f477

Only affects parts that relied on the linked list functionality of the GameMessageArguments

fix: revert accidental todo comment delete and add debug assert (TheS…

f627bba

…uperHackers#2630)

greptile-apps Bot reviewed May 11, 2026

View reviewed changes

Comment thread GeneralsMD/Code/GameEngine/Include/Common/MessageStream.h Outdated

Caball009 reviewed May 11, 2026

View reviewed changes

RikuAnt added 3 commits May 12, 2026 16:22

refactor: refactor auto and range based loops into c++98 compatible v…

84d7681

…ersions (TheSuperHackers#2630)

perf: reserve vector storage before iteratively populating and remove…

51bd443

… unnecessary test before uint conversion (TheSuperHackers#2630)

refactor: Make minor code review changes(TheSuperHackers#2630)

8e5eda3

- Return GameMessageArguments as const - Style fixes (remove blank space, indent and space out too packed methods) - Remove vector includes; already included from precompiled header

greptile-apps Bot reviewed May 12, 2026

View reviewed changes

Comment thread GeneralsMD/Code/GameEngine/Include/Common/MessageStream.h Outdated

refactor: refactor GameMessageArguments into const to avoid reinterpr…

6ca8832

…et_cast (TheSuperHackers#2630)

RikuAnt requested a review from Caball009 May 12, 2026 14:24

L3-M added Minor Severity: Minor < Major < Critical < Blocker ZH Relates to Zero Hour Refactor Edits the code with insignificant behavior changes, is never user facing Network Anything related to network, servers labels May 12, 2026

L3-M added this to the Code foundation build up milestone May 12, 2026

Caball009 reviewed May 13, 2026

View reviewed changes

Conversation

RikuAnt commented May 11, 2026 • edited by Caball009 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

TODO:

Uh oh!

greptile-apps Bot commented May 11, 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

Sequence Diagram

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Caball009 May 11, 2026

Choose a reason for hiding this comment

Uh oh!

RikuAnt May 12, 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!

Caball009 May 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!

Uh oh!

Caball009 May 13, 2026

Choose a reason for hiding this comment

Uh oh!

Caball009 May 13, 2026

Choose a reason for hiding this comment

Uh oh!

Caball009 May 13, 2026

Choose a reason for hiding this comment

Uh oh!

Caball009 May 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!

Caball009 May 13, 2026

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

RikuAnt commented May 11, 2026 •

edited by Caball009

Loading

greptile-apps Bot commented May 11, 2026 •

edited

Loading

RikuAnt May 12, 2026 •

edited

Loading

Caball009 May 13, 2026 •

edited

Loading

Caball009 May 13, 2026 •

edited

Loading

Caball009 commented May 13, 2026 •

edited

Loading