9

I have a class, PlayerInputComponent:

.h:

class PlayerInputComponent
{
public:
    PlayerInputComponent(PlayerMoveComponent& parentMoveComponent_, std::unique_ptr<IRawInputConverter> inputConverter_);
    PlayerInputComponent(PlayerInputComponent&& moveFrom);
    void update();

private:
    std::unique_ptr<IRawInputConverter> inputConverter;
    PlayerMoveComponent& parentMoveComponent;
};
}

.cpp:

PlayerInputComponent::PlayerInputComponent(PlayerMoveComponent& parentMoveComponent_, std::unique_ptr<IRawInputConverter> inputConverter_) :
    parentMoveComponent(parentMoveComponent_),
    inputConverter(std::move(inputConverter_))
{
}

PlayerInputComponent::PlayerInputComponent(PlayerInputComponent&& moveFrom) :
    parentMoveComponent(moveFrom.parentMoveComponent),
    inputConverter(moveFrom.inputConverter.release())
{
}

and a class, PlayerMoveComponent, that contains a PlayerInputComponent member and initializes it using a std::unique_ptr passed as a parameter. Its constructor:

PlayerMoveComponent::PlayerMoveComponent(/* other parameters */ std::unique_ptr<IRawInputConverter> inputConverter) :
    //other initializations
    inputComponent(PlayerInputComponent(*this, std::move(inputConverter)))
{
}

I defined my own move constructor for the PlayerInputComponent class since my understanding is that a default move constructor won't be constructed for a class which contains a reference member. In this case though I know that the reference will remain in scope for duration of the PlayerInputComponent object's lifetime.

Since I'm initializing the PlayerMoveComponent's inputComponent variable from a temporary, I believe one of the following two things is supposed to happen:

  1. PlayerInputComponent's move constructor is used to initialize the playerInputComponent member variable.
  2. The move is elided by the compiler.

However, Visual Studio 2012 spits this out:

error C2248: 'std::unique_ptr<_Ty>::unique_ptr' : cannot access private member declared in class 'std::unique_ptr<_Ty>'
1>          with
1>          [
1>              _Ty=SDLGame::IRawInputConverter
1>          ]
1>          c:\program files\microsoft visual studio 11.0\vc\include\memory(1447) : see declaration of 'std::unique_ptr<_Ty>::unique_ptr'
1>          with
1>          [
1>              _Ty=SDLGame::IRawInputConverter
1>          ]
1>          This diagnostic occurred in the compiler generated function 'PlayerInputComponent::PlayerInputComponent(const PlayerInputComponent &)'

Why is the copy constructor being called here? Making the PlayerInputComponent class's parentMoveComponent member a regular ParentMoveComponent instance, rather than a reference, gets rid of the error, but I don't understand why - I've tested and verified that move constructing objects with reference members works so long as you provide your own move constructor, so what's the deal?

Mykola
  • 3,343
  • 6
  • 23
  • 39
Benjamin Good
  • 201
  • 2
  • 4
  • @MooingDuck: I don't see how that answers the question. I'm pretty sure this is a bug in MSVC. – Jesse Good May 01 '13 at 00:03
  • 1
    @JesseGood: [oh you're right](http://coliru.stacked-crooked.com/), I misread part of the code. Then it's [This bug report of mine](https://connect.microsoft.com/VisualStudio/feedback/details/778513/msvc10-using-copy-constructor-instead-of-move) :P – Mooing Duck May 01 '13 at 00:15
  • The *declarations* of the aforementioned classes, stripped down to only include reproducible results (which doesn't appear to be too difficult) would be nice to see as well, specifically `PlayerMoveComponent` – WhozCraig May 01 '13 at 00:25
  • why either copy or move constructor should be called here, couldn't the compiler construct `inputComponent` using the constructor used in the initialization list – yngccc May 01 '13 at 00:25
  • @MooingDuck: I [think it's this one](http://connect.microsoft.com/VisualStudio/feedback/details/586332/default-copy-and-move-constructor-bug), the compiler shouldn't be generating the default copy ctor. – Jesse Good May 01 '13 at 00:32
  • What is the type of `PlayerMoveComponent::inputComponent` (the member mentioned in your constructor's initializer list)? I see `inputConverter` and `parentMoveComponent`, but not `inputComponent`. It could be that it's moving from the unique_ptr, but whatever conversion is happening between `PlayerInputComponent` and `decltype(PlayerMoveComponent::inputComponent)` requires a copy, which (of course) is invalid for unique_ptr. – Adam H. Peterson May 01 '13 at 00:34
  • 3
    @yngum: Assuming `inputComponent` is a `PlayerInputComponent`, then you told it to construct a `PlayerInputComponent` from the parameters, and then move construct `inputComponent` from that temporary. Presumably you meant `inputComponent(*this, std::move(inputConverter))` instead. – Mooing Duck May 01 '13 at 00:34
  • @MooingDuck I see, this is still compile error even though at runtime the compiler can optimize it out. – yngccc May 01 '13 at 00:45
  • @MooingDuck declaring a copy constructor seems to have fixed it, thanks! – Benjamin Good May 01 '13 at 02:05
  • @AdamH.Peterson, `inputComponent` is initialized with an rvalue, so it can use a move, it doesn't require a copy – Jonathan Wakely May 01 '13 at 08:01
  • 1
    @JonathanWakely, yes, it can move from the rvalue, but without knowing what type `inputComponent` actually is, I can't know that the rvalue can be used to initialize it without a copy being invoked. In particular, if `inputComponent` is of a class that contains a copyable-but-not-movable member, it will also (by default) be copyable-but-not-movable, meaning the move constructor will actually be a copy constructor. Similar side-effects can crop up depending on what constructors `inputComponent`'s type provides. That's why I think we need to know its type, and I don't see it in the OP. – Adam H. Peterson May 01 '13 at 22:50
  • @AdamH.Peterson, it's not a huge stretch of imagination to assume that something called `inputComponent` might be of type `PlayerInputComponent`, and we know that type has a move constructor because it's shown right there in the question. If it isn't of type `PlayerInputComponent` then the constructor invoked will be neither a copy ctor nor a move ctor, but a converting ctor from `PlayerInputComponent`. In any case, you said it _requires_ a copy, which is not true, you can say it might do a copy, but you can't claim to know it requires a copy, and common sense suggests it doesn't. – Jonathan Wakely May 01 '13 at 23:22
  • @JonathanWakely, since the OP is experiencing a problem he doesn't expect, I didn't want to assume anything. His type might be wrong or he may have intended to say "inputConverter" instead of "inputComponent" or any number of other details could be relevant. That's why I asked. And, the sentence where I said "requires" starts with "It could be that...". – Adam H. Peterson May 01 '13 at 23:35
  • @BenjaminGood I try to replicate your situation in my VS2012 and it's not just compiling but also it uses the move constructor as expected. Just to clarify, I made a forward declaration of the `PlayerMoveComponent` class, so I can use its reference in `PlayerInputComponent`. Then, as @JonathanWakely stated I need to presume that your `inputComponent` member was a `PlayerInputComponent`. Finally, when I call `PlayerMoveComponent( std::unique_ptr( new IRawInputConverter ) );` It does the job! Could you provide more info, I would like to see the error here? – Julio Raffaine May 03 '13 at 20:32

2 Answers2

2

If you initialize a new Object using =, the copy constructor will be triggered by default. To trigger the move constructor, you need to alter the behavior of operator= You can find an example here Hope I helped you.

Kostas Andrianos
  • 1,551
  • 2
  • 16
  • 21
1

I'm sorry in advance if this doesn't really answer your question, I just want to react on the apparent complexity of your problem. If I may, wouldn't this be a thousand times simpler:

/********************     **********     ********************/

class C {};
class B;



class A
{
public:

    A(): _b(nullptr), _c(nullptr) {}
    A( B *b, C *c ): _b(b), _c(c) {}
    A( A&& a ): _b(a._b), _c(a._c) {}

private:

    C *_c;
    B *_b;
};



class B
{
public:

    B( /* other parameters */ C *c ): _a( A(this,c) ) {}

private:

    A _a;
};


    /********************     **********     ********************/


int main()
{
    C c;
    B b(&c);
}

and yet achieve the same thing? I have nothing against using the new features in c++11, like std::unique_ptr, but IMHO, ensuring that a pointer can never be dereferenced from two places should not be a matter of run-time checking (except maybe in very rare cases), but a matter of design.. shouldn't it?

Jonathan H
  • 7,591
  • 5
  • 47
  • 80
  • I really agree with you, but I think the main reason to not use raw pointers is to have some security over who is using it. For instance in your code, I could pass a c pointer to your B class, and after this call delete on it! (BOOM ... it will crash sometime later). Using the unique_ptr as stated, would prevent this because the control would be passed to the new B instance and nobody outside of it could delete it. – Julio Raffaine May 07 '13 at 21:34
  • Ahn ... and by the way, I don't know if unique_ptr do any kind of check when dereferencing ... if you create a brand new one (nullptr) and try to access it, it will crash like any pointer. It's useful when you want to ensure who is the owner and it must be ONE owner (unique). – Julio Raffaine May 07 '13 at 21:38
  • @JulioRaffaine Thanks a lot for these comments! :) What I meant about dereferencing was this; the only reason you would want a `unique_ptr` is because you don't want anyone else to access it. AFA the deletion is concerned, funily enough, I think it should actually be simpler if you intended to use `unique_ptr` and that you eventually don't: the only class that contained the unique c-pointers here was A, so logically A should delete the memory allocated for c in its destructor, and "nullify" it instead when it is simply being copied. – Jonathan H May 07 '13 at 21:53