Skip to content

[GEN][ZH] Change TheGlobalData macro to variable in debug mode for debugging convenience #1054

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Caball009 wants to merge 5 commits into from
Closed

[GEN][ZH] Change TheGlobalData macro to variable in debug mode for debugging convenience #1054

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
94dd507
Changed TheGlobalData macro to variable for debugging convenience.
Caball009 Jun 11, 2025
c1cbbed
Simplified version using static union.
Caball009 Jun 11, 2025
9dda5be
Removed new line.
Caball009 Jun 11, 2025
6291ca0
Removed in-union initialization.
Caball009 Jun 11, 2025
131d412
Used old setup with extern.
Caball009 Jun 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 10 additions & 4 deletions Generals/Code/GameEngine/Include/Common/GlobalData.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -539,9 +539,15 @@ class GlobalData : public SubsystemInterface

};

// singleton
extern GlobalData* TheWritableGlobalData;

#define TheGlobalData ((const GlobalData*)TheWritableGlobalData)
#if defined(_DEBUG) || defined(RTS_DEBUG) || defined(RTS_INTERNAL)
static union
{
GlobalData* TheWritableGlobalData; ///< The global data singleton
const GlobalData* TheGlobalData; ///< Const shorthand for above singleton
};
#else
extern GlobalData* TheWritableGlobalData; ///< The global data singleton
#define TheGlobalData ((const GlobalData*)TheWritableGlobalData) ///< Const shorthand for above singleton
#endif

#endif
4 changes: 3 additions & 1 deletion Generals/Code/GameEngine/Source/Common/GlobalData.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,9 @@
#include "GameNetwork/FirewallHelper.h"

// PUBLIC DATA ////////////////////////////////////////////////////////////////////////////////////
GlobalData* TheWritableGlobalData = NULL; ///< The global data singleton
#if !defined(_DEBUG) && !defined(RTS_DEBUG) && !defined(RTS_INTERNAL)
GlobalData* TheWritableGlobalData = NULL; ///< The global data singleton
#endif

//-------------------------------------------------------------------------------------------------
GlobalData* GlobalData::m_theOriginal = NULL;
Expand Down
14 changes: 10 additions & 4 deletions GeneralsMD/Code/GameEngine/Include/Common/GlobalData.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -552,9 +552,15 @@ class GlobalData : public SubsystemInterface

};

// singleton
extern GlobalData* TheWritableGlobalData;

#define TheGlobalData ((const GlobalData*)TheWritableGlobalData)
#if defined(_DEBUG) || defined(RTS_DEBUG) || defined(RTS_INTERNAL)
static union
{
GlobalData* TheWritableGlobalData; ///< The global data singleton
const GlobalData* TheGlobalData; ///< Const shorthand for above singleton
Copy link

@xezon xezon Jun 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

If this works in Debug, then can we have this in Release too?

Copy link
Author

@Caball009 Caball009 Jun 12, 2025

Choose a reason for hiding this comment

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

Frankly, I'm not a great fan of this solution even though it seems quite neat. C++ has quite strict rules about unions:

It is undefined behavior to read from the member of the union that wasn't most recently written. Many compilers implement, as a non-standard language extension, the ability to read inactive members of a union.

https://www.cppreference.com/w/cpp/language/union.html

Now maybe in our case there are mitigating circumstances considering that both pointers are of the same type and only differ in const-ness, but I don't know. The idea was that the debugging convenience might outweigh the potential for UB, but I wouldn't want to do it this way in release mode.

Copy link
Author

@Caball009 Caball009 Jun 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

FWIW, my first commit is a setup with a second pointer like you suggested initially.

Copy link

@xezon xezon Jun 13, 2025

Choose a reason for hiding this comment

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

Hmm unfortunate. Can we put

extern GlobalData* TheWritableGlobalData;
extern const GlobalData* TheGlobalData;

on the same cache line somehow?

Copy link
Author

@Caball009 Caball009 Jun 13, 2025

Choose a reason for hiding this comment

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

I think putting their declarations right under each other, as they are, is the best we can do in that regard.

What's the relevance of cache lines here?

Copy link

@helmutbuhler helmutbuhler Jun 17, 2025

Choose a reason for hiding this comment

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

How about we replace TheWritableGlobalData with TheGlobalData and make it writeable. The whole const thing is stupid, let's not make it even more stupid.

Copy link

@xezon xezon Jun 17, 2025

Choose a reason for hiding this comment

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

Yes sure. We can also protect against accidental writes by writing expression like so:

if (0 == TheGlobalData->m_value)

Copy link
Author

@Caball009 Caball009 Jun 18, 2025

Choose a reason for hiding this comment

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

Yoda notation isn't great for readability imo and modern compilers can detect and warn for accidental assignments in if statements.

I think there's value in applying const as much as possible and doubly so when accessing a singleton, so I wouldn't be in favor of removing it entirely. I'm on the fence with regards to the change to a template. One fewer macro is a good thing, having to change 800+ cases of TheWritableGlobalData less so. I'd probably just stick to the first commit implementation.

Copy link

@helmutbuhler helmutbuhler Jun 18, 2025

Choose a reason for hiding this comment

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

Never heard it called Yoda notation before :)

Copy link

@xezon xezon Jun 18, 2025

Choose a reason for hiding this comment

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

I'd probably just stick to the first commit implementation.

It is mostly useless though. I almost always test in Release.

};
#else
extern GlobalData* TheWritableGlobalData; ///< The global data singleton
#define TheGlobalData ((const GlobalData*)TheWritableGlobalData) ///< Const shorthand for above singleton
#endif

#endif
4 changes: 3 additions & 1 deletion GeneralsMD/Code/GameEngine/Source/Common/GlobalData.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,9 @@
#include "GameNetwork/FirewallHelper.h"

// PUBLIC DATA ////////////////////////////////////////////////////////////////////////////////////
GlobalData* TheWritableGlobalData = NULL; ///< The global data singleton
#if !defined(_DEBUG) && !defined(RTS_DEBUG) && !defined(RTS_INTERNAL)
GlobalData* TheWritableGlobalData = NULL; ///< The global data singleton
#endif

//-------------------------------------------------------------------------------------------------
GlobalData* GlobalData::m_theOriginal = NULL;
Expand Down
Loading