1

I've been working at this all day so I hope I don't forget any important details, but here goes. My original goal was to have a player factory that encapsulated the logic of how to create a player.

Here's what that looks like:

Player PlayerFactory::CreatePlayer() {

    Player constructee(id_generator++);

    // c++1x move semantics says this isn't a copy!
    return constructee;
}

Within the player, there is a composite PlayerInputComponent member variable. This PlayerInputComponent object encapsulates the logic of dealing with player input, to do this it needs a reference/pointer to the actual player itself. Easy enough, it takes the reference to the player in as it's only parameter to it's constructor.

When the player object is constructed, it's initializer list passes a reference to itself to the PlayerInputComponent object. Here's how it looks:

Player::Player(const unsigned int id) 
    : id(id), 
    input_component(*this)
{
...
}

I understand that de-referencing this within an initializer list is usually a bad idea, but I'm just using it to set a reference within the PlayerInputComponent object. Here's the constructor:

PlayerInputComponent::PlayerInputComponent(Player &player) 
    : player_entity(player) { // set the reference, it is never used within the constructor body
}

For whatever reason, when the player factory returns the local copy of the player it has created, the references get garbled. My intent, was for the instance of the player created on the stack to be moved and assigned to the callee. This looks like the following:

auto player = player_factory.CreatePlayer();

After the code executes the player's composite PlayerInputComponent object's reference to it's player is mangled. I understand that the move constructor for the player is invoked when the factory returns the local player object to the callee, but what happens the reference of the player within the PlayerInputComponent? Is there a better way to solve this problem? I like the semantic meaning of having a reference vs a pointer in this situation, although I did try it with a pointer and get the same thing.

Can anyone explain to me what is happening to the PlayerInputComponent's reference to the player when the player object is moved out of the CreatePlayer() member function and assigned to "auto player"?

For completeness here are the object's declarations:

class PlayerInputComponent {

private:
    Player &player_entity;
    void HandleKeyboardInput();
public:
    //PlayerInputComponent(PlayerInputComponent &&other);
    //PlayerInputComponent(const PlayerInputComponent &other);
    PlayerInputComponent(Player &player);
    void Update();
};

And here is the Player:

class Player : public Entity{
    friend class PlayerFactory;
private:
    const unsigned int id;

public:
    Player(const unsigned int id);

    // components
    PlayerInputComponent input_component;
};

I've reconstructed a small example below that shows the exact behavior, which compiles/demonstrates the problem. Thank you!

class B;

class A  {
public:
    A(B& bb) : b(bb) { }

private:
    B &b;
};

class B {
public:
    B() : a(*this) { othercrap = othercrap2 = 0; }
    int othercrap;
    int othercrap2;
private:
    A a;
};

class BFactory {
public:
    B CreateB() {
    B temp;
        return temp;
    };
};

B start() {
    BFactory fac;
    auto bi = fac.CreateB();

    return bi;
}

int main(int argc, char *argv[]) {
    auto i = start();
}
Short
  • 7,767
  • 2
  • 25
  • 33
  • You should rethink the whole "I like the semantic meaning of having a reference vs a pointer"... the differences between `T* const` and `T&` are *very* few actually. – Ben Voigt Feb 20 '12 at 03:47

2 Answers2

5

Moving invalidates references. (A reference is made to a storage location, not an object)

Remember that moving creates a new object (hence the name, move constructor). Ownership of the contents is transferred, but the object is not actually moved to a new location in memory. It is destroyed after the new object is constructed.

You should define Player's copy and move constructors to properly bind the reference in the new object.

Ben Voigt
  • 277,958
  • 43
  • 419
  • 720
  • I wrote the following move constructor, Player(Player &&other) : id(other.id), input_component(other.input_component) { }; It seems the reference is not being transferred correctly, is this not correct? – Short Feb 20 '12 at 21:52
  • 1
    @shortstompcpp: Well, that binds `input_component`'s internal reference to `other`, which is being destroyed. Instead use `: id(other.id), input_component(*this)`. Maybe even `input_component(*this, other.input_component)` (and write the corresponding constructor) if there's any other state to be copied/moved. – Ben Voigt Feb 20 '12 at 22:25
  • Aha! I understand now. Works perfectly! Thanks! I also understand why what I was doing had no possible chance of working. – Short Feb 21 '12 at 02:50
1

I can't see any move semantics at your sample. CreateB will just get that internal temporary (temp) which will be trivially copied on the temporary object of return (the xvalue) to be assigned to bi. Your references get garbled because they are trivially copied references to already destroyed local temporary objects.

oblitum
  • 11,380
  • 6
  • 54
  • 120
  • The local variable is an *xvalue* in the return statement, so it should be moved. `class Player` has an implicitly defaulted move constructor. But moving vs copying makes no difference, a new object is still being created in either case, and the reference is left dangling. – Ben Voigt Feb 20 '12 at 03:43
  • @BenVoigt Hey, I've heard of lvalues and rvalues but what's a xvalue?? to me, as he don't use std::move, and temp is a ordinary locally declared variable, it's a lvalue... – oblitum Feb 20 '12 at 03:53
  • and as a lvalue it'll get copied. – oblitum Feb 20 '12 at 04:01
  • You need to understand *xvalues* to make sense of move. See section 3.10 of the C++11 Standard. And [this question](http://stackoverflow.com/q/3601602/103167) – Ben Voigt Feb 20 '12 at 04:06
  • @BenVoigt Cool, so from that definition I've read, the temporary object that lasts in place of *fac.CreateB* is *the* xvalue, but *temp*, without the std::move is still a lvalue and will get trivially garbled copied to this xvalue. – oblitum Feb 20 '12 at 04:16
  • 1
    `temp` is an *lvalue*. Inside the return statement, it is also an *xvalue*. And it will be moved to the actual return value, which is also an *xvalue* and will in turn be moved to `bi`. `bi` is an *lvalue*, and becomes an *xvalue* in the return statement of `start()`. It is *moved* to the return value, and the return value (a temporary and therefore *xvalue*) is moved to the `i` variable inside `main()`. And by "moved", I mean used as the source variable passed to a move constructor. – Ben Voigt Feb 20 '12 at 04:24
  • @BenVoigt wow... this is new to me, now I need to make sense of all those `return std::move(var)` I've seen so far... – oblitum Feb 20 '12 at 04:29
  • let us [continue this discussion in chat](http://chat.stackoverflow.com/rooms/7929/discussion-between-ben-voigt-and-chico) – Ben Voigt Feb 20 '12 at 04:31
  • Thanks @BenVoigt, now I see *temp* gets *trivially garbled moved*. good references: [1](http://stackoverflow.com/q/9152798/1000282), [2](http://stackoverflow.com/a/3601661/1000282) – oblitum Feb 20 '12 at 05:06