How does wrapping a pointer work with rvalue references?

Question

std::shared_ptr::operator* returns by lvalue reference, and the answer given on overloading pointer like operations here says that the convention is to return by lvalue reference. However, when I'm using the following code, I get error C2664: 'AdjacencyList::addVertex' : cannot convert parameter 1 from 'AdjacencyList::vertex_type' to 'AdjacencyList::vertex_type &&': You cannot bind an lvalue to an rvalue reference:

std::shared_ptr<vertex_type> AdjacencyList::addVertex(vertex_type&& v)
{
    auto existingVertex(findVertex(v));

    if (!existingVertex.isValid())
    {
        existingVertex = std::make_shared<vertex_type>(std::forward<vertex_type>(v))
        m_vertices.push_back(existingVertex);
    }

    return existingVertex;
};

AdjacencyList minimumSpanningTree;
// startVertex is a shared_ptr to a vertex returned from a previous call of addVertex
// on another AdjacencyList object
const auto mstStartVertex(minimumSpanningTree.addVertex(*startVertex));

Should I provide AdjacencyList::addVertex(const vertex_type& v) or change the code at the bottom of the above block to make a copy of the vertex before passing to addVertex?

AdjacencyList minimumSpanningTree;
Vertex s(*startVertex);
const auto mstStartVertex(minimumSpanningTree.addVertex(std::move(s)));

score 2 · Answer 1 · answered Apr 10 '13 at 05:23

2

I would think that you should return a copy from your operator*, as the sematics of the std::weak_ptr suggest that you can not guarantee that a returned reference would stay valid. Since the returned copy is then given to a function which can move it somewhere else, it should also be efficient enough, since addVertex looks like it would require a copy anyways, i.e., if you would create an overload of addVertex, it will create a copy of the passed const reference internally, would it?

answered Apr 10 '13 at 05:23

Daniel Frey

55,810
13
122
180

It wouldn't necessarily make a copy of the vertex; currently I look to see if the same vertex has already been added and if it has I do nothing. In most cases, it would copy though. – masrtis Apr 10 '13 at 05:36
After your update, I'm not sure if I understand the need for the `weak_ptr` or the wrapper. Why don't you just return a `std::shared_ptr`? – Daniel Frey Apr 10 '13 at 05:40
If the AdjacencyList goes out of scope or the vertex gets deleted but someone is holding onto a vertex, I don't want to keep the vertex information around. The best example, though not relevant to my particular use case I can think of is scheduling pathfinding for multiple agents running across multiple frames in a game with a dynamic navigation graph. Maybe storing unique_ptr in AdjacencyList and returning an index would be better? – masrtis Apr 10 '13 at 05:55
I think you make the wrong method responsible for this. Return a `std::shared_ptr` and let the caller decide if he wants to use it directly or if he want to store it. The caller should then store a `weak_ptr` if it is possible that the pointer-to object becomes invalid and if the caller could handle the situation of `.lock()` returning empty. – Daniel Frey Apr 10 '13 at 06:03
That sounds like a reasonable way to go. Since std::shared_ptr::operator* still returns by reference, I've edited the question a bit to use shared_ptr specifically. – masrtis Apr 10 '13 at 07:35

score 1 · Accepted Answer · edited May 23 '17 at 11:43

The most efficient approach in terms of redundant copies is to provide rvalue and const reference overloads:

std::shared_ptr<vertex_type> AdjacencyList::addVertex(vertex_type&&);
std::shared_ptr<vertex_type> AdjacencyList::addVertex(const vertex_type&);

To eliminate the redundant code, you can forward to a template method or to a concrete method taking a bool flag and performing const_cast as appropriate.

If the overhead of copying the Vertex object is minimal compared to the cost of increased code, and if the if block is usually or often entered, then the redundant copy will make your code clearer. Your second suggested call will work better if you just create a prvalue temporary that doesn't need to be moved:

const auto mstStartVertex(minimumSpanningTree.addVertex(Vertex{*startVertex}));

However in that case you might as well create the temporary in the call itself, by providing a single value overload (How to reduce redundant code when adding new c++0x rvalue reference operator overloads):

std::shared_ptr<vertex_type> AdjacencyList::addVertex(vertex_type);

How does wrapping a pointer work with rvalue references?

2 Answers2