Just turned in an assignment where I ran into this. It's not really a big issue, but I'm wondering if there's a better way to deal with it than what I did.

I have a class 'Game', which has a few members that need to refer back to the owning instance of Game.

In order to do this, the classes for the member objects each have a member: 'Game& game'. Obviously I need to assign this in the initializer list for those classes, which I do. The problem is that this creates a circular dependency between these classes and Game.

Example:

class Game {
public:
  Game();
private:
  Thingy thing;
};

class Thingy {
public:
  Thingy(Game& game);
private:
  Game& game;
};

Game::Game() : thingy(*this) { // <--- Not allowed, since 'this' is not defined yet.
}

Thingy::Thingy(Game& game) : game(game) {
}

So I ended up using std::unique_ptr to store those objects and I just allocate them in the ctor body of 'Game'.

Is there a cleaner way to do this? I suppose I could set up some kind of an event system, but that would be overkill for the small assignments I'm doing, and it means doing an awful lot of legwork for something so simple.

Is there a cleaner way to do this?

First of all: If you absolutely must do this then no, not really but I can pretty much guarantee you there is a better way of structuring your code. You should consider avoiding the cyclic dependency altogether. Such a simple loop might seem innocuous at first but over time, in a larger project this can get you into serious trouble and is a nightmare to untangle.

Second of all: using std::unique_ptr is going to get you intro trouble. Game contains an instance of Thingy and Thingy contains a std::unique_ptr<Game> to the Game instance that owns it. When Game's destructor goes off the destructor for Thingy will go off as well triggering the destructor for the std::unique_ptr<Game> instance which will in turn cause the destructor for Game to go off. Stack overflow. If you are new-ing an instance of std::unique_ptr<Game> instide of Thingy as your phrasing suggest the stack won't overflow but there is little point in having a pointer to a smart pointer...all the advantages are lost, you might as well use a regular pointer.

[edit] Forgot to mention, if you do new std::unique_ptr<Game> in Thingy you need to delete it or you will leak memory but doing so will cause the Game and Thingy destructors to call each other recursively until the stack overflows. You just can't win.

Yes this is allowed, since 'this' IS defined AND initialized, but 'Game' isn't fully constructed yet, so you have to make sure Thingy's constructor doesn't actually use it yet - but the address is valid, so Thingy's constructor can save it for later use. In this simple example, Thingy isn't actually using the pointer/reference in the constructor, so it's cool to do.

You can get it to work, you can even do so without breaking the implementation out into multiple files; however, from the MSDN:

The this pointer is valid only within nonstatic member functions. It cannot be used in the initializer list for a base class.

The base-class constructors and class member constructors are called before this constructor. In effect, you've passed a pointer to an unconstructed object to another constructor. If those other constructors access any members or call member functions on this, the result will be undefined. You should not use the this pointer until all construction has completed.

It triggers a warning that most developers elevate to an error. My understanding is that the C++ language specification doesn't insist that 'this' be defined until the class's user defined constructor enters scope but it just so happens that the Visual Studio implementation defines it before churning through the initializer list so you can get off with a warning.

Is there a cleaner way to do this?

First of all: If you absolutely must do this then no, not really but I can pretty much guarantee you there is a better way of structuring your code. You should consider avoiding the cyclic dependency altogether. Such a simple loop might seem innocuous at first but over time, in a larger project this can get you into serious trouble and is a nightmare to untangle.

Second of all: using std::unique_ptr is going to get you intro trouble. Game contains an instance of Thingy and Thingy contains a std::unique_ptr<Game> to the Game instance that owns it. When Game's destructor goes off the destructor for Thingy will go off as well triggering the destructor for the std::unique_ptr<Game> instance which will in turn cause the destructor for Game to go off. Stack overflow. If you are new-ing an instance of std::unique_ptr<Game> instide of Thingy as your phrasing suggest the stack won't overflow but there is little point in having a pointer to a smart pointer...all the advantages are lost, you might as well use a regular pointer.

[edit] Forgot to mention, if you do new std::unique_ptr<Game> in Thingy you need to delete it or you will leak memory but doing so will cause the Game and Thingy destructors to call each other recursively until the stack overflows. You just can't win.

Game::Game() : thingy(*this) { // <--- Yes this is allowed

Yes this is allowed, since 'this' IS defined AND initialized, but 'Game' isn't fully constructed yet, so you have to make sure Thingy's constructor doesn't actually use it yet - but the address is valid, so Thingy's constructor can save it for later use. In this simple example, Thingy isn't actually using the pointer/reference in the constructor, so it's cool to do.

I recommend a solution of lazy or two-step initialization. I also recommend that internally it follows a pimpl idiom.


Why does it absolutely need a reference? Is there a reason that a pointer will not work? And why does it absolutely need it during construction? My guess is that it doesn't NEED either. Make it a pointer that is initialized to null. Later, after the other parts are set up, set the pointer on every object that will need it.

Extending that further you could also allow both. As an obvious example within C++ think of the file stream library. You can use the constructors that automatically open a file. You can also use the constructors that don't, then open a file later. The same with your code, you can provide a version that accepts the parameter, and also provide a constructor that doesn't require the parameter and lets you set it later.

In our current engine at work there is actually a 4-step game object initialization process. The game object sees a constructor which is nearly always empty. Then an OnCreate() method is called where you know all the base member variables are created and initialized; here you set up your own values but do not touch other components, the function is skipped if the object is being loaded through serialization. Next an OnStartup() method where you register with other components and recreate any non-persisted values, this is run both after deserialization and on new object creation. Finally, when the object is placed in the world, OnPlacedInWorld(), only a few objects need this additional step, but sometimes they need to hook up with nearby objects and this provides that opportunity.

Ah, cool. Okay.

Yeah, it is dangerous, but it can be used in this case.

However, I went outside to smoke just now and realized that this whole situation is resulting from inappropriate intimacy between the objects. They should be communicating their state to Game through getters and letting Game handle the interactions rather than interacting with one another. I'm going to try to adjust it and see if I can get rid of the references to game.

Edit:

Oooooooh yeah. That's so much cleaner. I'm gonna resubmit this one.

frob, on 24 Aug 2013 - 01:34 AM, said:

In our current engine at work there is actually a 4-step game object initialization process. The game object sees a constructor which is nearly always empty. Then an OnCreate() method is called where you know all the base member variables are created and initialized; here you set up your own values but do not touch other components, the function is skipped if the object is being loaded through serialization. Next an OnStartup() method where you register with other components and recreate any non-persisted values, this is run both after deserialization and on new object creation. Finally, when the object is placed in the world, OnPlacedInWorld(), only a few objects need this additional step, but sometimes they need to hook up with nearby objects and this provides that opportunity.

Holy crap, man!

Couldn't you just create separate ctors for normal construction and deserialization construction? I mean, I get the OnPlacedInWorld() part, but it sounds like OnCreate() and OnStartup() could just be normal ctors that call a common member function after the serializable values are set. Why do you guys avoid using the ctor? Or, rather, if it needs to be plugged in this way, why not a pair of factory functions?

This is pretty standard. It's not that you can't achieve the same with multiple constructors but breaking it up into multiple function calls or passes allows a few different things:

1) External systems can run code in-between the initialization passes. This is almost a necessity, especially for games.

2) Code reuse. Often times the default constructor does stuff that all the other initialization functions want done. If everything was broken out into multiple, overloaded constructors then a lot of code would get copied across them.

3) Clear intent. OnPlacedInWorld() OnCreate() and OnStartup() tells would be modifiers of the class where to put various bits of initialization code while the systems using the class know when to call what.

4) Performance. As mentioned, the minimally filled out default constructor allows faster serialization as you can avoid doing things that will simply be undone by the serializer.