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

Question

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();
}

Answer 1

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?)