Constructors, static variables, and confusion...

Started by
7 comments, last by ApochPiQ 12 years ago
I use a static array within a class. It's purpose is to store pointers to all instances of that class when they are created. This has worked for me without any complications for many years. Here is the offending class in its entirety:


class CNetworkCommand
{
public:
// TODO: move to network.cpp
CNetworkCommand(TNetworkCommandFuntion pfnFunction) : m_pfnFunction(pfnFunction), m_ulUniqueID(m_ulStaticCounter)
{
assert(m_pfnFunction != 0);
m_ulStaticCounter++;
m_CmdPtrs.Add(this);
}
~CNetworkCommand(void)
{
m_CmdPtrs.RemoveItem(this);
}
inline void Execute(int iIndex, void *pData) const { m_pfnFunction(iIndex, pData); }
inline unsigned long GetID(void) const { return m_ulUniqueID; }
static CNetworkCommand* GetCmdByID(const unsigned long ulID)
{
// TODO: fails here because there are only 6 commands in m_CmdPtrs... there are 11 commands... none are being destroyed... WTF
for(unsigned long i = 0; i < m_CmdPtrs.GetSize(); i++)
{
if(m_CmdPtrs->GetID() == ulID)
return m_CmdPtrs;
}
return 0;
}
private:
TNetworkCommandFuntion m_pfnFunction;
const unsigned long m_ulUniqueID;
static CArray<CNetworkCommand*> m_CmdPtrs;
static unsigned long m_ulStaticCounter;


CNetworkCommand objects are declared as static variables in two separate files: server.cpp and client.cpp. My problem occurs during the construction of these variables. The static variables in server.cpp are initialized first and m_CmdPtrs stores them just fine. When the first variable in client.cpp is initialized, it is also added to the array just fine, except that the array has magically been reset (as if its constructor had been called), and all the previously stored variables are gone.

What would cause the static array to reset itself like this once a variable from another file is constructed?
Advertisement
You've probably been hit by what's often called the static initialization order fiasco.

Also, pay attention to the rule of three. The fact that you don't have a copy constructor that's storing 'this' may be causing problems.
I originally ran into this problem when I started using this technique and found this same FAQ while searching for a solution. All of my CNetworkCommand variables are wrapped inside of their respective classes (CServer and CClient). I have no global variables in my program. I also use this exact same technique with my console variables and console commands (storing their "this" pointers to a static CArray variable) and have never had a problem with them since.

I have made a copy constructor and assignment constructor for this class but I get the same result, which is that m_CmdPtrs (which is a static variable in CNetworkCommand) is reset to an empty array once the constructor for a CNetworkCommand object, that is located in a separate file, is called. How can the constructor for a static variable within a class be called twice? Why does it not do this with my console commands and console variables? They are set up, quite literally, the exact same way!

Here is the class with the copy constructor and assignment operator:


class CNetworkCommand
{
public:
// TODO: move to network.cpp
CNetworkCommand(TNetworkCommandFuntion pfnFunction) : m_pfnFunction(pfnFunction), m_ulUniqueID(m_ulStaticCounter)
{
assert(m_pfnFunction != 0);
m_ulStaticCounter++;
m_CmdPtrs.Add(this);
}
CNetworkCommand(const CNetworkCommand &src) : m_pfnFunction(src.m_pfnFunction), m_ulUniqueID(src.m_ulUniqueID) { }
~CNetworkCommand(void)
{
m_CmdPtrs.RemoveItem(this);
}
inline void Execute(int iIndex, void *pData) const { m_pfnFunction(iIndex, pData); }
inline unsigned long GetID(void) const { return m_ulUniqueID; }
static CNetworkCommand* GetCmdByID(const unsigned long ulID)
{
// TODO: fails here because there are only 6 commands in m_CmdPtrs... there are 11 commands... none are being destroyed... WTF
for(unsigned long i = 0; i < m_CmdPtrs.GetSize(); i++)
{
if(m_CmdPtrs->GetID() == ulID)
return m_CmdPtrs;
}
return 0;
}
CNetworkCommand &operator = (const CNetworkCommand &src)
{
if(this == &src)
return *this;
m_pfnFunction = src.m_pfnFunction;
m_ulUniqueID = src.m_ulUniqueID;
return *this;
}
private:
TNetworkCommandFuntion m_pfnFunction;
unsigned long m_ulUniqueID;
static CArray<CNetworkCommand*> m_CmdPtrs;
static unsigned long m_ulStaticCounter;
}; // CNetworkCommand
Your copy constructor isn't correct, as it's not adding 'this' to m_CmdPtrs. Presumably it also wants to bump m_ulStaticCounter;

Also, putting static variables in a class isn't a solution to the static initialization order fiasco. Read the FAQ again and make sure you understand it. If not, feel free to ask questions here.

One solution to the SIOF to put each static variable in its own function and have the function return a reference to it.


class CNetworkCommand
{
// ...

private:
static CArray<CNetworkCommand *> &CommandPointers()
{
static CArray<CNetworkCommand *> cmdPtrs;
return cmdPtrs;
}
};


And now use CommandPointers() everywhere you used to use m_CmdPtrs. You'll have to do a similar thing with m_ulStaticCounter.

However, I think the best solution is to either avoid keeping a global collection of objects altogether, or delay any manipulation of the global list until after main() has started i.e. avoid relying on static initialization tricks.
Thanks for the reply, I am going to go over the FAQ a little slower and try to see where i mis-interpreted it. I already see that I completely got the idea of wrapping statics in a function wrong so I am sure I missed something else. I will try your tip out after I re-read the FAQ and let you know where I stand.
I changed the class a little bit...


class CNetworkCommand
{
public:
// TODO: move to network.cpp
CNetworkCommand(TNetworkCommandFuntion pfnFunction) : m_pfnFunction(pfnFunction)
{
assert(m_pfnFunction != 0);
m_ulUniqueID = StaticCounter()++;
CommandPointers().Add(this);
}
CNetworkCommand(const CNetworkCommand &src) : m_pfnFunction(src.m_pfnFunction)
{
assert(m_pfnFunction != 0);
m_ulUniqueID = StaticCounter()++;
CommandPointers().Add(this);
}
~CNetworkCommand(void)
{
CommandPointers().RemoveItem(this);
}
inline void Execute(int iIndex, void *pData) const { m_pfnFunction(iIndex, pData); }
inline unsigned long GetID(void) const { return m_ulUniqueID; }
static CNetworkCommand* GetCmdByID(const unsigned long ulID)
{
for(unsigned long i = 0; i < CommandPointers().GetSize(); i++)
{
if(CommandPointers()->GetID() == ulID)
return CommandPointers();
}
return 0;
}
CNetworkCommand &operator = (const CNetworkCommand &src)
{
if(this == &src)
return *this;
m_pfnFunction = src.m_pfnFunction;
m_ulUniqueID = src.m_ulUniqueID;
return *this;
}
private:
static CArray<CNetworkCommand*> &CommandPointers(void)
{
static CArray<CNetworkCommand*> a;
return a;
}
static unsigned long &StaticCounter(void)
{
static unsigned long a;
return a;
}
TNetworkCommandFuntion m_pfnFunction;
unsigned long m_ulUniqueID;
}; // CNetworkCommand



Now it works as intended. I am so confused because I assumed the error was in my code. I used this trick in two other classes with no problems at all but I guess the third time was the charm... the compiler finally was not on my side. After reading and completely understanding the FAQ you linked, I think it is about time I come up with a better way of tracking these variables. Thanks so much for your help!
If you decide to keep your static members, it's probably a good idea to give m_ulStaticCounter the same treatment.

An observation: presumably each instance should have a distinct value for m_ulUniqueID? This invariant isn't upheld in your assignment operator; you'll end up with two object with equivalent IDs.
I was actually thinking a lot about that as well. Suppose I wanted to do something like "netcmd1 = netcmd2"... I would want netcmd1 to essentially become a netcmd2. When I call netcmd1->Execute (after the assignment), it would be expected that it runs the netcmd2 function. This is why I decided to copy the m_ulUniqueID instead of declaring a new one. m_ulUniqueID is what is used to execute the correct function (its basically unique to each function "registered" in the constructor).

I had to declare a new m_ulUniqueID in the copy constructor since I had no choice but to add a new "this" pointer to the static array there (necessary because it is removed from the array in the destructor). Changing m_ulUniqueID in the assignment operator would essentially cause an error because that m_ulUniqueID will not match the m_ulUniqueID of any CNetworkCommand in the array, and therefore the correct command will not be found when I call GetCmdByID();

It is amazing how what seemed like a very simple class is becoming so complicated. In the meantime, I will take your advice and apply this same treatment to all static variables that come into play before main().
Please don't mark threads "solved."


Thanks!

Wielder of the Sacred Wands
[Work - ArenaNet] [Epoch Language] [Scribblings]

This topic is closed to new replies.

Advertisement