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

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. ;)