Is it better to pass by value for moveable types instead of overloading functions?

Question

I want to reduce copying to a minimum, especially for expensive types/classes. So I could define overloads like this:

void SetParams(const std::map<int, std::string> &params) { mParams = parameters; }
void SetParams(std::map<int, std::string> &&params) { mParams = std::move(params); }
void SetBoardInfo(const DataInfoStruct &board_info) { mBoardInfo = board_info; }
void SetBoardInfo(DataInfoStruct &&board_info) { mBoardInfo = std::move(board_info); }

in a class that has a std::map member variable called mParams and a DataInfoStruct struct member called mBoardInfo.

Since DataInfoStruct has no user defined constructors or destructors I assume that it has an implicit move constructor.

Then I can invoke either one of the overloads with an lvalue or rvalue. However, I could just have one version of each function like so:

void SetParams(std::map<int, std::string> params) { mParams = std::move(params); }
void SetBoardInfo(DataInfoStruct board_info) { mBoardInfo = std::move(board_info); }

In the second way I will always create a copy because I am passing by value, but then I move the parameter into the member variable. If I pass an rvalue I do no copying at all, but for an lvalue there will be one copy made always - which I will do any way if I used the first method.

Which of the two methods is recommended? Are rvalue references best used only for constructors and assignment operators?

related/maybe dupe: https://stackoverflow.com/questions/57923963/am-i-using-stdmove-too-often — NathanOliver, Sep 24 '19 at 15:31
You could write book on this... Here is starting point from a Herb Sutter talk: https://youtu.be/xnqTKD8uD64?t=3849 I think this is too broad for SO though. — Holt, Sep 24 '19 at 15:32
@NathanOliver The linked question seems to only be related to constructor, for which you likely have different recommendations that, e.g., setters method (see Herb's talk in my previous comment). — Holt, Sep 24 '19 at 15:33

eerorika · Answer 1 · 2019-09-24T17:44:25.897

2

In the second way I will always create a copy because I am passing by value

No. The argument can be move constructed from an xvalue or initialised directly from a prvalue (latter only since C++17; prior to that the move may be elided as an optimisation). So, even through you create a distinct object (as far as the abstract machine is concerned), there isn't necessarily a copy.

In case of an lvalue, the argument will be copied; just like it will be copied in option 1, in which case it will call the lvalue reference overload.

Which of the two methods is recommended?

I recommend option 2 (passing by value) by default. It halves the amount of boilerplate code, and the performance difference between a single copy + single move versus single copy is usually insignificant.

An exception to this is objects that are slow to move, such a std::array with many elements. I recommend option 1 (reference overloads) if you've measured a bottleneck caused by option 2, and found that 1 is faster.

edited Sep 24 '19 at 17:44

answered Sep 24 '19 at 15:40

eerorika

232,697
12
197
326

That is what I said. The second method only copies if I pass by lvalue reference. – jignatius Sep 24 '19 at 17:41
@jignatius *"The second method only copies if I pass by lvalue reference."*. You don't pass by reference in the second method at all. You pass by value. – eerorika Sep 24 '19 at 17:43
Agree. I pass by value (creating a copy) using a lvalue. – jignatius Sep 24 '19 at 17:50
@jignatius In the part that I quoted, you stated that a copy will always be created. This is incorrect. The statements that follow that are correct. – eerorika Sep 24 '19 at 17:51
For an rvalue no copy is made. – jignatius Sep 24 '19 at 17:53
@jignatius Indeed. That contradicts with what you state in the quoted part of the question. – eerorika Sep 24 '19 at 17:53
Ok. Sorry. My terminology was a bit wrong. I really meant pass by value but use an lvalue - which does create a copy. – jignatius Sep 24 '19 at 17:55

Aykhan Hagverdili · Accepted Answer · 2019-09-24T16:24:42.380

2

The first version is usually marginally faster but it introduces code duplication. The second way is recommended for objects that are cheap to move. Since most objects are cheap to move in C++, the second way is usually fine. But, there is one more alternative:

template<typename T>
void SetParams(T&& params) { mParams = std::forward<T>(params); }

template<typename T>
void SetBoardInfo(T&& board_info) { mBoardInfo = std::forward<T>(board_info); }

Thanks to universal references, this is the best of both worlds if it is OK to have templated setters.

edited Sep 24 '19 at 16:24

answered Sep 24 '19 at 16:18

Aykhan Hagverdili

28,141
6
41
93

1

exactly what I had in my mind – fadedreamz Sep 24 '19 at 16:24
Only downside is as it is now templated, you can pass funny stuff with it. If you want to also stop that, maybe enable_if required for more rigid type checking – fadedreamz Sep 24 '19 at 16:31
I don't want to use templates here because the parameters cannot be anything. – jignatius Sep 24 '19 at 17:43
@jignatius it is possible to use `enable_if` to make your own type checking. – Aykhan Hagverdili Sep 24 '19 at 17:49
@jignatius have a look at [is_convertible](https://en.cppreference.com/w/cpp/types/is_convertible) and [enable_if](https://en.cppreference.com/w/cpp/types/enable_if) – Aykhan Hagverdili Sep 24 '19 at 17:51
You summed up the different approaches well. Thanks. – jignatius Sep 26 '19 at 03:22

Ayjay · Answer 3 · 2019-09-24T17:13:34.327

The general recommendation is the first way. The second way forces an allocation to be made, whereas that is not always necessary. For an example:

#include <vector>
#include <iostream>

bool my_log = false;

struct my_alloc : std::allocator<int> {
    int* allocate(std::size_t n) {
        if (my_log) std::cout << "allocate: " << n << '\n';
        return std::allocator<int>::allocate(n);
    }

    template<typename U>
    struct rebind {
        using other = my_alloc;
    };
};

using test_vec = std::vector<int, my_alloc>;
struct foo {
    test_vec v_;
    void set_value(test_vec v) { v_ = v; }
    void set_ref(const test_vec& v) { v_ = v; }
    void set_ref(test_vec&& v) { v_ = v; }
};

int main() {
    test_vec long_vec = { 0,1,2,3,4 };
    test_vec short_vec = { 0,1,2 };
    foo f;
    foo g;
    my_log = true;
    std::cout << "value\n";
    f.set_value(long_vec);
    f.set_value(short_vec);
    f.set_value(short_vec);
    f.set_value(short_vec);

    std::cout << "reference\n";
    g.set_ref(long_vec);
    g.set_ref(short_vec);
    g.set_ref(short_vec);
    g.set_ref(short_vec);
}

This produces the output:

value
allocate: 5
allocate: 5
allocate: 3
allocate: 3
allocate: 3
reference
allocate: 5

As you can see, under some circumstances, passing by value requires an extra allocation every single time, which may be avoided if you simply pass by reference.

The long story short, you have to write out a set(const X&) function and a set(X&&) version every time. There's no way to shortcut it.

You don't need an allocation if your current member has enough storage. That's why the second version is bad - even if your current object has sufficient capacity, you're forcing a new object to be allocated anyway. — Ayjay, Sep 24 '19 at 15:43
I agree on this, but if this is what you want to say you should badly edit that answer because it's clearly incomplete and no one is going to deduce this by reading it. — Holt, Sep 24 '19 at 15:44
That's an interesting answer. However in your example you are not invoking set_ref(test_vec&& v) because you are not using std::move or using a temporary/rvalue. — jignatius, Sep 25 '19 at 04:56
@jignatius The goal here is not to show the benefit of `set_ref(test_vec)` against `set_ref(test_vec&&)`, but rather to show that if you use `set_ref(test_vec)` + `move`, you always have to construct a new object, and then move from it, which always include an allocation for a `std::vector` (construction), while with `set_ref(test_vec const&)`, you are only assigning, and allocation is not required if the vector you're assigning to is large enough. — Holt, Sep 25 '19 at 07:36

score 0 · Answer 4 · edited Sep 24 '19 at 17:33

Premature optimization is the root of all evil.

I want to reduce copying to a minimum, especially for expensive types/classes.

Don't. Unless those functions are called millions or billions of times, or your parameters map has millions of elements - this is unlikely to take up a significant amount of time. From the looks of it, you are prematurely optimizing.

If any of those two cases hold - which I very much doubt - you are losing a whole lot more time, and possibly space, by using an std::map - since it's extremely inefficient. And a map of individually-heap-allocated strings, no less!

So, just go with:

void SetParams(std::map<int, std::string> params) : mParams(parameters) { }

and let the compiler take care of it.

PS - You should always measure before going to any effort for optimization, and specifically before you prefer supposed performance benefits over simplicity, readability, DRY (don't repeat yourself) and so on.

Is it better to pass by value for moveable types instead of overloading functions?

4 Answers4

Premature optimization is the root of all evil.