perf(gamemessage): Replace GameMessageArgument linked lists with vectors

[RikuAnt]

Fixes #2630

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.

[greptile-apps]

Greptile Summary

This PR replaces the manual linked-list argument storage (m_argList/m_argTail/m_argCount/m_next) in GameMessage and NetGameCommandMsg with std::vector, eliminating pointer threading and simplifying all related construction, iteration, and destruction code.

• Core refactor (MessageStream.h/.cpp, NetCommandMsg.h/.cpp): linked-list fields and walk loops are replaced with std::vector<GameMessageArgument*>; getArgument/getArgumentDataType become O(1) index lookups; a DEBUG_ASSERTCRASH is added to guard the 255-argument limit.
• Recorder.cpp (both variants): dead commented-out code is removed and the argument iteration loop is updated to use the new vector-based API.
• Not yet replicated in Generals/ per the PR TODO — the Generals copies are updated here but the PR notes this remains a follow-up item.

Confidence Score: 4/5

Safe to merge once the assert placement in allocArg() is fixed; all other changes are straightforward refactoring.

In allocArg(), the DEBUG_ASSERTCRASH fires after the 256th element has already been pushed into the vector. If execution continues (release build or IDE continue), getArgumentCount() returns static_cast<UnsignedByte>(256) which wraps to 0 — every downstream loop over arguments silently processes nothing. Moving the check before push_back makes it a real guard. The rest of the refactor is correct and cleaner than the original linked-list code.

MessageStream.cpp in both Generals/ and GeneralsMD/ — specifically the allocArg() function where the assert ordering needs to be corrected.

Important Files Changed

Filename	Overview
GeneralsMD/Code/GameEngine/Source/Common/MessageStream.cpp	Refactors list-walking loops to direct vector index access; DEBUG_ASSERTCRASH guard for the 255-arg limit is placed after push_back, making it a post-hoc diagnostic rather than a true guard — the violating element is already committed before the assertion fires.
Generals/Code/GameEngine/Source/Common/MessageStream.cpp	Mirror of GeneralsMD MessageStream.cpp; same misplaced DEBUG_ASSERTCRASH issue.
GeneralsMD/Code/GameEngine/Include/Common/MessageStream.h	Replaces linked-list members with std::vector<GameMessageArgument*>; getArgumentCount() now derives the count from the vector size but still returns UnsignedByte, which silently truncates if size ever exceeds 255.
Generals/Code/GameEngine/Include/Common/MessageStream.h	Mirror of the GeneralsMD header change: same std::vector replacement and same getArgumentCount() truncation concern.
Core/GameEngine/Include/GameNetwork/NetCommandMsg.h	Removes the four old linked-list fields and adds std::vector<GameMessageArgument*> m_argList; adds forward declaration for GameMessageArgument. Clean change.
Core/GameEngine/Source/GameNetwork/NetCommandMsg.cpp	Constructor/destructor and addArgument rewritten to use vector push_back and index iteration; reserve() call added in copy-constructor for efficiency. No issues found.
GeneralsMD/Code/GameEngine/Source/Common/Recorder.cpp	Removes commented-out dead code and rewrites the arg iteration loop; introduces a local argType that shadows the outer parser-type variable (style-only concern).
Generals/Code/GameEngine/Source/Common/Recorder.cpp	Mirror of GeneralsMD Recorder.cpp; same minor variable-shadowing nit.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["appendXxxArgument()"] --> B["allocArg()"]
    B --> C{Before PR: linked-list append}
    B --> D{After PR: vector push_back}
    C --> E["m_argTail->m_next = newArg / m_argCount++"]
    D --> F["m_argList.push_back(arg) then DEBUG_ASSERTCRASH(size <= 255)"]
    F -->|"assert fires AFTER push"| G["256th element already in vector"]
    G --> H["getArgumentCount() returns UnsignedByte(256) = 0"]

    subgraph Readers
        I["getArgument(i)"] -->|"O(n) walk to O(1) index"| J["m_argList[i]->m_data"]
        K["getArgumentDataType(i)"] -->|"O(n) walk to O(1) index"| L["m_argList[i]->m_type"]
        M["~GameMessage()"] -->|"pointer chase to index loop"| N["deleteInstance(m_argList[i])"]
    end

Loading

Prompt To Fix All With AI

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

---

### Issue 1 of 4
GeneralsMD/Code/GameEngine/Source/Common/MessageStream.cpp:108-115
The `DEBUG_ASSERTCRASH` check fires AFTER `push_back`, so the 256th argument is already in the vector when the assertion triggers. If execution continues (e.g. in a debug session, or in any release build where the assert compiles to nothing), `getArgumentCount()` returns `static_cast<UnsignedByte>(256)` which wraps to 0 — callers that loop `for (i = 0; i < msg->getArgumentCount(); ++i)` will silently iterate over zero entries. Moving the check before `push_back` makes it an enforceable guard rather than a post-hoc diagnostic. The same issue exists in `Generals/Code/GameEngine/Source/Common/MessageStream.cpp`.

```suggestion
	DEBUG_ASSERTCRASH(
		m_argList.size() < 255,
		("If a GameMessage needs more than 255 arguments, it needs to be split up into multiple GameMessage's.")
	);
	GameMessageArgument *arg = newInstance(GameMessageArgument);
	m_argList.push_back(arg);
	return arg;
```

### Issue 2 of 4
Generals/Code/GameEngine/Source/Common/MessageStream.cpp:108-115
Same assert-after-push issue as in the GeneralsMD copy: the 256th argument is already committed to the vector before the assertion fires, and `getArgumentCount()` will silently return 0 (via `UnsignedByte` truncation) if execution continues past the assert.

```suggestion
	DEBUG_ASSERTCRASH(
		m_argList.size() < 255,
		("If a GameMessage needs more than 255 arguments, it needs to be split up into multiple GameMessage's.")
	);
	GameMessageArgument *arg = newInstance(GameMessageArgument);
	m_argList.push_back(arg);
	return arg;
```

### Issue 3 of 4
GeneralsMD/Code/GameEngine/Source/Common/Recorder.cpp:792-796
The loop-local `argType` (`GameMessageArgumentDataType`) shadows the outer `argType` (`GameMessageParserArgumentType*`) declared a few lines above. The outer is exhausted before the loop, so there is no behavioural bug, but the shadowing will produce compiler warnings and is easy to resolve with a distinct name.

```suggestion
	for (size_t i = 0; i < argsCount; ++i) {
		GameMessageArgumentDataType argDataType = msg->getArgumentDataType(i);
		const GameMessageArgumentType* arg = msg->getArgument(i);
		writeArgument(argDataType, *arg);
	}
```

### Issue 4 of 4
Generals/Code/GameEngine/Source/Common/Recorder.cpp:790-794
Same variable-shadowing nit as in the GeneralsMD copy: the loop-local `argType` shadows the `GameMessageParserArgumentType* argType` from the enclosing scope.

```suggestion
	for (size_t i = 0; i < argsCount; ++i) {
		GameMessageArgumentDataType argDataType = msg->getArgumentDataType(i);
		const GameMessageArgumentType* arg = msg->getArgument(i);
		writeArgument(argDataType, *arg);
	}
```

_{Reviews (9): Last reviewed commit: "refactor: mirror Zero Hour changes to ge..." | Re-trigger Greptile}

[Caball009]

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.

[xezon]

Is Caballs review addressed?

[RikuAnt]

Addressed now @xezon Also included the mirrored changes into Generals/ side

[xezon]

I recommend to replicate to Generals not before approval.

[xezon]

Looks good to me. Needs to be replicated to Generals.

Maybe in a next refactor we could also change

std::vector<GameMessageArgument*>

to

std::vector<GameMessageArgument>

because why do we need GameMessageArgument to be allocated like that? I am not sure, but looks unnecessary.

[RikuAnt]

Thanks for thorough codereview @Caball009 & @xezon Very insightfull and helpfull on the coding practices in the repo 🙏

Changes replicated to Generals now

[greptile-apps]

Want your agent to iterate on Greptile's feedback? Try greploops.

[xezon]

Bot is complaining about the 255 limit assert. Fix:

return std::min<UnsignedByte>(255, m_argList.size());

[RikuAnt]

The bot is hallucinating, this is the exact behaviour as it was before (UnsignedByte counter that would wrap back to 0), but with actual debug crash if it actually can happen.

Using min logic would potentially mask the issue more when it happens, the current logic would keep it more obvious. The current functionality is broken in any case, as we don't handle the more than 255 arguments case at all.

Also do we want to keep retail compatibility? I think this would cause mismatch with retail as the message args would end up differing

[xezon]

There is no message that has anywhere near 255 arguments. So it is impossible to break retail compat with this.

I think the complaint is valid. If the value wraps, it better not lose visibility to all arguments, but simply be unable to use the excess ones beyond 255. The assert will highlight that too many arguments are added.

[RikuAnt]

Given that no existing message type can approach 255 arguments, the min clamping would never actually do anything. The debug crash in allocArg() already covers the case if that assumption ever breaks during development, so I think that's sufficient. The min doesn't fix the root cause either - if too many args were ever added, the message would still be corrupt, just silently. I wouldn't make this change.

[xezon]

ok