Skip to content

fix(memory): Fix memory leak for RecorderClass::m_crcInfo#2713

Open
Caball009 wants to merge 5 commits into
TheSuperHackers:mainfrom
Caball009:fix_memory_leak_crcInfo
Open

fix(memory): Fix memory leak for RecorderClass::m_crcInfo#2713
Caball009 wants to merge 5 commits into
TheSuperHackers:mainfrom
Caball009:fix_memory_leak_crcInfo

Conversation

@Caball009
Copy link
Copy Markdown

This PR fixes a memory leak for RecorderClass::m_crcInfo. I decided to fix this one by not allocating the CRCInfo object dynamically. This did require a larger change of moving the class to the header, though.

See commits for clean diffs.

TODO:

  • Replicate in Generals.

@Caball009 Caball009 added Minor Severity: Minor < Major < Critical < Blocker Gen Relates to Generals ZH Relates to Zero Hour Memory Is memory related Fix Is fixing something, but is not user facing labels May 14, 2026
@greptile-apps
Copy link
Copy Markdown

greptile-apps Bot commented May 14, 2026

Greptile Summary

This PR fixes a memory leak in RecorderClass by changing m_crcInfo from a heap-allocated CRCInfo* to a value-type member CRCInfo. To support default initialization, a no-arg constructor was added to CRCInfo, and the class definition was moved from the .cpp file into the RecorderClass definition in the header.

  • CRCInfo is now a nested protected class inside RecorderClass in Recorder.h, with a default constructor that zero-initialises all fields.
  • All m_crcInfo->member() call sites are updated to m_crcInfo.member(), and the heap allocation in playbackFile() is replaced with a value assignment — correctly destroying any previous queued CRC state on each new playback.

Confidence Score: 5/5

Safe to merge. The conversion from heap pointer to value member is mechanically straightforward, and repeated-playback state is correctly cleared by the move-assignment in playbackFile().

The change is tightly scoped: a class definition is moved to the header, a default constructor is added, and all access sites are updated from pointer to value. The compiler-generated move-assignment on std::list correctly drains any queued CRCs on each new playback, and the default constructor initialises all fields to well-defined values. No logic paths were altered.

No files require special attention.

Important Files Changed

Filename Overview
GeneralsMD/Code/GameEngine/Include/Common/Recorder.h CRCInfo moved here as a nested protected class with a new default constructor; m_crcInfo changed from pointer to value member.
GeneralsMD/Code/GameEngine/Source/Common/Recorder.cpp CRCInfo class definition and file-local implementation removed; moved to header/scoped definitions. All pointer dereferences updated to value access.

Reviews (3): Last reviewed commit: "Made 'CRCInfo' class part of 'RecorderCl..." | Re-trigger Greptile

@Caball009 Caball009 force-pushed the fix_memory_leak_crcInfo branch from 282478b to c5e3079 Compare May 14, 2026 19:54
Comment thread GeneralsMD/Code/GameEngine/Include/Common/Recorder.h Outdated
This avoids an unnecessary dynamic memory allocation and fixes a memory leak.
Comment thread GeneralsMD/Code/GameEngine/Include/Common/Recorder.h Outdated
Comment thread GeneralsMD/Code/GameEngine/Include/Common/Recorder.h Outdated
Comment thread GeneralsMD/Code/GameEngine/Source/Common/Recorder.cpp
Comment thread GeneralsMD/Code/GameEngine/Include/Common/Recorder.h Outdated
Copy link
Copy Markdown

@xezon xezon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good. Needs replicate to Generals.

int GetQueueSize() const { return m_data.size(); }

UnsignedInt getLocalPlayer() { return m_localPlayer; }

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can remove the empty lines here for compactness.

// compare those with the local gamestate, they wouldn't sync up.
// So, in order to fix this, we need to queue up our local CRCs,
// so that we can check it with the crc messages that come later.
// This class is basically that queue.
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While at it, perhaps compact the format of the massive comment a bit.

#endif

RecorderClass::CRCInfo::CRCInfo() :
m_sawCRCMismatch(FALSE),
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While at it, prefer format like:

RecorderClass::CRCInfo::CRCInfo()
	: m_sawCRCMismatch(FALSE)
	, m_skippedOne(FALSE)


class CRCInfo;

class RecorderClass : public SubsystemInterface {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While at it

class RecorderClass : public SubsystemInterface
{

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Fix Is fixing something, but is not user facing Gen Relates to Generals Memory Is memory related Minor Severity: Minor < Major < Critical < Blocker ZH Relates to Zero Hour

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants