Is it bad practice to move an l-value reference parameter?

Question

I originally assumed that it is bad practice to move an l-value reference parameter. Is this indeed commonly agreed by the C++ developer community?

When I call a function that has an R-value reference parameter, it's clear that I must expect that the passed object can be moved. For a function that has an L-value reference parameter, this is not so obvious (and before move semantics were introduced with C++11, it wasn't possible at all).

However, some other developers I recently talked to don't agree that moving l-value references shall be avoided. Are there strong arguments against it? Or is my opinion wrong?

Since I was asked to provide a code example, here is one (see below). It's an artificial example just for demonstrating the issue. Obviously, after calling modifyCounter2(), a call of getValue() will cause a segmentation fault. However, if I were a user of getValue() without knowing its internal implementation, I would be very surprised. If, on the other hand, the parameter were an R-value reference, I would be totally clear that I should not use the object anymore after calling modifyCounter2().

class Counter
{
public:
    Counter() : value(new int32_t(0))
    {
        std::cout << "Counter()" << std::endl;
    }

    Counter(const Counter & other) : value(new int32_t(0))
    {
        std::cout << "Counter(const A &)" << std::endl;

        *value = *other.value;
    }

    Counter(Counter && other)
    {
        std::cout << "Counter(Counter &&)" << std::endl;

        value = other.value;
        other.value = nullptr;
    }

    ~Counter()
    {
        std::cout << "~Counter()" << std::endl;

        if (value)
        {
            delete value;
        }
    }

    Counter & operator=(Counter const & other)
    {
        std::cout << "operator=(const Counter &)" << std::endl;

        *value = *other.value;

        return *this;
    }

    Counter & operator=(Counter && other)
    {
        std::cout << "operator=(Counter &&)" << std::endl;

        value = other.value;
        other.value = nullptr;

        return *this;
    }

    int32_t getValue()
    {
        return *value;
    }

    void setValue(int32_t newValue)
    {
        *value = newValue;
    }

    void increment()
    {
        (*value)++;
    }

    void decrement()
    {
        (*value)--;
    }

private:
    int32_t* value;      // of course this could be implemented without a pointer, just for demonstration purposes!
};

void modifyCounter1(Counter & counter)
{
    counter.increment();
    counter.increment();
    counter.increment();
    counter.decrement();
}

void modifyCounter2(Counter & counter)
{
    Counter newCounter = std::move(counter);
}

int main(int argc, char ** argv)
{
    auto counter = Counter();

    std::cout << "value: " << counter.getValue() << std::endl;

    modifyCounter1(counter);  // no surprises

    std::cout << "value: " << counter.getValue() << std::endl;

    modifyCounter2(counter);  // surprise, surprise!

    std::cout << "value: " << counter.getValue() << std::endl;

    return 0;
}

Welterweight

I for one would be very surprised if my object was moved if I passed it by lvalue reference. Generally one should not violate the principal of least surprise and moving from an lvalue reference does just that. – Adhern

An example would help (at least me) to understand your thought process better. – Yb

@Adhern In general, I don't see any surprise if the called function modifies the state of an object passed by reference (non const). – Bataan

Show code which demonstrates this potential bad practice. I'm not 100% sure what is your problem. – Signal

void foo(const std::string& arg) { bar( std::move(arg) ) } will call a copy constructor and move a temporary object, this is generally legal but have no sense, because it negate the copy elision. – Soulsearching

Generally, I would not move a non-const l-value reference parameter. If that was my intent, I'd either have the parameter be pass-by-value (and then move from that value) or r-value reference (which makes the caller use std::move if it is named, or ready-to-use if it is from an unnamed temporary). – Gibert

If move constructor leave the moved object in valid state, it might be ok. It is not the case here. – Brelje

@Victor: That's clear. I didn't mean const references. My question refers to non-const L-value references. – Welterweight

@Eljay: Yes, that's exactly what I also thought. But at least some people who answered here seem to have a different opinion. – Welterweight

@A.Schorr - what is the reason of this move? first call to modifyCounter2 will bring to ~Counter() and as well as to the delete value; – Soulsearching

@VictorGubin No it won't – Atharvaveda

Sample was very helpful – Signal

@Jarod42: If I understand you correctly, you think that moving an L-value reference parameter is principally OK, but the implementation of the Counter class is wrong? -- I think the move-constructor itself is ok, but I would have to add a nullptr-check in the other member function (such as getValue) to avoid the crash. But then the questions arises which value should getValue() return? This is just an artifical example, but I think in general it can be sometimes difficult to implement all member function in a sensible way so that they still so non-surprising things after moving. – Welterweight

@VictorGubin: As I already wrote: this is an artificial example just for explaining my thoughts. Of course, the implementation of modifyCounter2() has no practical sense. – Welterweight

“C makes it easy to shoot yourself in the foot; C++ makes it harder, but when you do it blows your whole leg off.” ― Bjarne Stroustrup ` – Soulsearching

IMO it doesn't matter. if you accept an non const reference then you are telling the caller the value you pass in can change. Whether that's because it was moved or assigned makes no difference. You have to know what the likely change is to call the function in the first place. – Double

If Counter(Counter && other) was value = other.value; other.value = new int; instead, then modifyCounter2 is like writing void Counter::reset() – Cryptogram

@BiagioFesta The surprise comes from not the modification, but from the fact the object could be put into an unspecified state. If we did this with a vector for instance, then you can't call front on the vector afterwards. – Adhern

The bottom line is a non-const parameter can change value. It is the function's responsibility to make sure the new value makes sense to the caller. That new value can be the result of a move because the caller should not care how the new value is arrived at. – Double

@Adhern There are also things you can not do with a vector that changed value by other means. You can't re-use iterators for example. – Double

@A.Schorr: moving a l-value might be OK, but implementation of Counter is wrong, not really for getValue (as state above, vector::front cannot be called from empty vector), but you no longer can do thing to reenable counter, such as counter = anothercounter. – Brelje

@Adhern Sure, but even without moving anything the problem remains. The callee may just do vector.clear(), even in that case, you cannot use vector.front() afterward (just for example). Or just vector.push_back will lead all caller iterators into invalid state (even worse of moving IMHO)! Of course, my comments here are quite general. – Bataan

Maybe I should reformulate my original question: If I move a parameter of a function, are there situations in which it is better to define this parameter as an (non-const) L-value reference parameter instead of an R-value reference or a by-value parameter? – Welterweight

Answer 1

Yes, that is surprising and unconventional.

If you want to permit a move, the convention is to have pass by value. You can std::move from inside the function as appropriate, while the caller can std::move into the argument if they decide they want to forfeit ownership.

This convention is where the notion to name std::move that way came from: to promote explicit forfeiture of ownership, signifying intent. It's why we put up with the name even though it's misleading (std::move doesn't really move anything).

But your way is theft. ;)

Atharvaveda