Nonconst lvalue reference

Question

I am creating an undirected graph an every time I add a neighbour A to a node B I have to add node B as a neghbour of A as well but my approach does not work.

Non-const lvalue reference to type 'Element *' cannot bind to a temporary of type 'Element *'

class Element
{
    std::vector<Element *> m_neighbours;
private:

public:
    void addNeighbour(Element*& neighbour)
    {
        m_neighbours.push_back(neighbour);
        neighbour->addNeighbour(this);
    }
};

What is happening?
Best way to solve it?

What's the motivation to `emplace_back` a pointer rather than just `push_back` it? — Wyck, Apr 06 '20 at 18:21
Passing the pointer by reference also seems like a code smell to me, especially since it is not changing within the function. — 0x5453, Apr 06 '20 at 18:22
But if I do not pass it by reference in theorey I have a copy so when calling addNeighbour I would not be calling the real neighbour isnt it? — Hector Esteban, Apr 06 '20 at 18:24
@HectorEstebanCabezos You will have a copy *of the pointer*, not a copy of *the thing being pointed at.* — templatetypedef, Apr 06 '20 at 18:27

score 1 · Answer 1 · answered Apr 06 '20 at 18:25

To understand what the issue is, let's imagine that you wrote this code instead:

void addNeighbour(Element*& neighbour)
{
    m_neighbours.emplace_back(neighbour);
    neighbour->addNeighbour(this);
    neighbour = nullptr; // <--- This is new
}

Now, think about what happens when you make this call:

neighbour->addNeighbour(this);

The call to the function passes in this by reference, meaning "please, feel free to reassign this." And then in the function call, the last line, indeed, tries to reassign neighbour to nullptr. But that's a problem, because you can't write

this = nullptr; // Error!

because this is an rvalue.

The easiest fix here would be to not take the parameter by reference, since you aren't doing anything to it where you'd actually need the reference. Just take in an Element*, saying "please hand me a copy of the pointer you're interested in."

(Independently - your code will get you into trouble because calling A->addNeighbour(B) will call B->addNeighbour(A), which calls A->addNeighbour(B), which calls B->addNeighbour(A), etc. until you blow out your call stack. You should add a check in here to make sure that, if the Element already is recorded, you don't add it a second time. You may want to make m_neighbours a std::unordered_set rather than a std::vector for that purpose.)

When you are trying to change what the pointer points to inside a function, you might take a reference to it. Although you also would want a good reason to not just use a return instead. — Zuodian Hu, Apr 06 '20 at 18:33

score 1 · Answer 2 · answered Apr 06 '20 at 18:26

The this pointer is defined to be a prvalue, and your function takes an lvalue. The language forbids that sort of binding for various reasons. You're not modifying the given pointer, so just pass it by value instead of by reference.

void addNeighbour(Element* neighbour);

instead of

void addNeighbour(Element*& neighbour);

Nonconst lvalue reference

2 Answers2