2

I am in the process of converting some legacy code to take advantage of smart pointers in C++. However, I ran into a runtime issue that I'm struggling to figure out. I have code such as the following:

struct foo {
    int field;
};

class SomeClass {
private:
    std::unique_ptr<foo> m_Foo;
    int m_Field;
public:
    SomeClass(std::unique_ptr<int> ctorArg)
        : m_Field(ctorArg->field), m_Foo(std::move(ctorArg)) {
    }
};

std::unique_ptr<foo> fooPtr{ new foo() };
auto const classInstance{ std::make_unique<SomeClass>(std::move(fooPtr)) };

If I put a breakpoint prior to calling the SomeClass constructor, I can verify that fooPtr is not null. However, once I am within the SomeClass constructor, the application is crashing when trying to initialize m_Field, since ctorArg is null (empty). Is this expected? If I change the constructor signature to take std::unique_ptr<int> &&, I see the same issue. Can someone please explain what the issue is with this code, and how to go about fixing it?

Jeff G
  • 4,470
  • 2
  • 41
  • 76
  • 2
    You can only have one unique_ptr (one ownership). When you move it, you are passing this ownership forward, so the behind is assigned as nullptr – Amadeus Mar 13 '19 at 16:31
  • 1
    https://stackoverflow.com/questions/8114276/how-do-i-pass-a-unique-ptr-argument-to-a-constructor-or-a-function/8114913 – Mat Mar 13 '19 at 16:35
  • Unfortunately your code has two variables called `ctorArg` so it's not completely clear which you are talking about. If you are talking about the second occurance then that is expected behaviour for `unique_ptr`. – john Mar 13 '19 at 16:35
  • I have updated the code to use separate variable names, since @Amadeus comment indicated that the original wording of this question was unclear about the behavior it exhibited. – Jeff G Mar 13 '19 at 16:38
  • Declare SomeClass constructor as public. Else code is ok. – seccpur Mar 13 '19 at 16:43
  • @Mat I fail to see how that link is relevant. Is my code in any way violating the recommendations? It appears to me I am 100% compliant with the recommendations in that post, yet the code isn't working. – Jeff G Mar 13 '19 at 16:44
  • From @Mat link, "After newBase is constructed, nextBase is guaranteed to be empty. You don't own the object, and you don't even have a pointer to it anymore. It's gone. This is ensured because we take the parameter by value. std::move doesn't actually move anything; it's just a fancy cast. std::move(nextBase) returns a Base&& that is an r-value reference to nextBase. That's all it does." – Juan Carlos Ramirez Mar 13 '19 at 16:45
  • @JuanCarlosRamirez Yes, I understand that. But the problem is that the parameter is null by the time it reaches my constructor, NOT that the `intPtr` variable is null after the call to my constructor. It appears that `std::make_unique` is eating the pointer enroute to my constructor. Is that expected? – Jeff G Mar 13 '19 at 16:48
  • 1
    @AhmedMasud no, `std::unique_ptr intPtr { new int() };` creates a dynamic `int`, and initialises `intPtr`. Your snippet isn't valid C++ – Caleth Mar 13 '19 at 16:49
  • Cannot reproduce the issue. Dies the program (*not your debugger*) behave differently from what you expect? Please post a [mcve]. – n. m. could be an AI Mar 13 '19 at 16:59
  • @n.m. You are correct. This code doesn't reproduce the issue. I am looking into what is different in my actual use case so I can get real help. I think it has something to do with the class I am passing not having a move constructor, but am looking into it. Standby... – Jeff G Mar 13 '19 at 17:02
  • I finally figured it out... it was a field order initialization issue (trying to use the passed `ctorArg` after it was moved, due to initialization being done in the order of declaration within the class). I have updated the question to reflect the issue more accurately. – Jeff G Mar 13 '19 at 18:04
  • Hopefully a lesson in pushing a MCVE up-front!! – Lightness Races in Orbit Mar 13 '19 at 18:16
  • BTW my compiler has had warnings for this since 1066 AD! Do you have warnings turned off? Why? – Lightness Races in Orbit Mar 13 '19 at 18:16
  • @LightnessRacesinOrbit I just built the above example with the "Warning Level" set to "All" in VS2017u2, and although I got 606 warnings, not one of them was for out-of-order initialization. Still, your comment made me think of release notes in old English, and I immediately thought of the holy hand grenade user instructions. Thou shalt not initialize the second declared member variable within a constructor, unless thou immediately preceedith such declaration by initialization of the first declared member variable. I want a Monty Python C++ user's guide in the worst way. – Jeff G Mar 13 '19 at 18:37
  • I suggest warning level 2. Warning level 4 will give you craptons of spam from their own standard library implementation, go figure. – Lightness Races in Orbit Mar 14 '19 at 00:21

1 Answers1

2

The order of member-initializer-list is irrelevant, the members will be initialized in the order of their declaration.

So, first m_Foo(std::move(ctorArg)) will zero out ctorArg then m_Field(ctorArg->field) will attempt to derefence an empty ctorArg.

Change your code to:

class SomeClass {
private:
    std::unique_ptr<foo> m_Foo;
    int m_Field;
public:
    SomeClass(std::unique_ptr<int> ctorArg)
        : m_Foo(std::move(ctorArg)), m_Field(m_Foo->field) {
    }
};

That is, always mention initializers in the order the fields are declared, and don't use input arguments which have been moved-out from.

rustyx
  • 80,671
  • 25
  • 200
  • 267