CODEWITHSUNDEEP
c++referenceinitialization
EricAtAIR
EricAtAIR

Reputation: 83

Passing stack allocated value by reference to a constructor in C++

I'm working with an external library and there's some code that gives me pause. Basically, there's a vector that is allocated inside a loop (on the stack). That vector is then passed by reference to the constructor of some object, and used to initialize one of the object's vector fields, which was not declared as a reference. Is the newly created object holding a reference to something that no longer exists? Or is this just a more efficient way of copying the vector, in which case the fact that it was allocated on the stack makes no difference?

Here's a minimal example:

class Holder {
public:
    Holder(vector<int>& vref) : vec(vref) {}
    vector<int> vec;
}

Holder* MakeHolder() {
    vector<int> v {1, 2};
    return new Holder(v);
}

int main() {
    Holder *h = MakeHolder();
}

Upvotes: 3

Views: 1047

Answers (4)

Lightness Races in Orbit
Lightness Races in Orbit

Reputation: 385114

There's no reference held to a departed object, but I certainly wouldn't call it "efficient". Without an std::move in that ctor-initialiser, the vector must be copied.

You could put std::move in there but then Holder would be a little confusing to use.

Personally I'd take the vector in by value, so the calling scope can std::move into it (or pass a temporary which will do this automatically), then std::move the constructor argument into the new member. That way you literally just have one vector the entire time.

class Holder {
public:
    Holder(vector<int> vref) : vec(std::move(vref)) {}
    vector<int> vec;
}

Holder* MakeHolder() {
    vector<int> v {1, 2};
    return new Holder(std::move(v));  // Or just `return new Holder({1,2});`
}

int main() {
    Holder *h = MakeHolder();
}

And, this way, if you want to keep the original vector alive (not moved-from) then that's fine too! Just pass it in and it'll get copied. Things will "just work" without really needing to know what's inside the constructor code (you only need to know that it takes a value).

The other thing I'd change is introducing std::unique_ptr, because you currently have a memory leak:

class Holder {
public:
    Holder(vector<int> vref) : vec(std::move(vref)) {}
    vector<int> vec;
}

std::unique_ptr<Holder> MakeHolder() {
    return std::make_unique<Holder>({1,2});
}

int main() {
    auto h = MakeHolder();
}

(Some people would spell MakeHolder()'s return type auto, but not me. I think it's important to know what you're going to get. For example, otherwise you have to read the code to know what the result's ownership semantics are! Is it a raw pointer? Something else?)

Upvotes: 4

Slava
Slava

Reputation: 44238

Is the newly created object holding a reference to something that no longer exists?

No, copy constructor called for initialization of member variable vec

Or is this just a more efficient way of copying the vector, in which case the fact that it was allocated on the stack makes no difference?

Yes, but common practice is to use const reference to avoid copy ctor on passing argument by value:

Holder(cont vector<int>& vref) : vec(vref) {}

so maybe somebody made a mistake and missed const or had some other reason not to use const reference here.

Note: with move semantics passing object by (const) reference in some cases could be even less efficient than passing by value.

Upvotes: 0

NathanOliver
NathanOliver

Reputation: 180500

Is the newly created object holding a reference to something that no longer exists?

No. Holder stores the vector by value so it's vector and the function local one are different objects.

Or is this just a more efficient way of copying the vector, in which case the fact that it was allocated on the stack makes no difference?

Yes. if

Holder(vector<int>& vref) : vec(vref) {}

had been

Holder(vector<int> vref) : vec(vref) {}

Then you would first have to copy the vector into vref and then you would have to copy vref into vec. By taking a reference you save that first copy.

Another way to do it would be

Holder(const vector<int>& vref) : vec(vref) {}
// or
Holder(vector<int> vref) : vec(std::move(vref)) {}

which allows you to accept lvalues and rvalues.

Upvotes: 4

bipll
bipll

Reputation: 11940

Its member variable is a vector, not a reference, so it is a Holder's own copy (and each std::vector privately owns its elements.)

Upvotes: 0

Related Questions