2

I'm trying to change my code to take a vector by value using std::move instead of passing it by reference because I've gathered that would be more efficient. I've seen different ways of doing this though, one having the constructor pass by value and using std::move in the constructor, and the other way being to initialize the class with an std::move and having the constructor take an rvalue (did I get that right?). Some example below:

method 1:

Constructor:

StatisticsCompiler::StatisticsCompiler(std::vector<Wrapper<StatisticsMC>> Inner_) :Inner(std::move(Inner_))
{
}

in main:

vector<Wrapper<StatisticsMC>> gathererArray{ meanGatherer, absQuantileGatherer, relVaRGatherer, relESGatherer };
StatisticsCompiler gathererCombiner(gathererArray);

method 2.

Constructor:

StatisticsCompiler::StatisticsCompiler(std::vector<Wrapper<StatisticsMC>>&& Inner_) :Inner(Inner_)
{
}

main:

vector<Wrapper<StatisticsMC>> gathererArray{ meanGatherer, absQuantileGatherer, relVaRGatherer, relESGatherer };
StatisticsCompiler gathererCombiner(std::move(gathererArray));

Is there a difference between what's going on here or is it the same thing, the first method "looks" nicer in main but the second method is the way I would intuitively understand it to work from learning about rvalues. If performance wise they're the exact same, then what's standard practice?

dandan78
  • 13,328
  • 13
  • 64
  • 78
Oscar
  • 279
  • 1
  • 10
  • 1
    I prefer the second -- explicit -- version. I like to see explicit constructor/function signatures to understand what's going on. It makes it easier to reason about the code. – jvd Jun 02 '20 at 18:20
  • Pass by value and move in the constructor implementation, because then users don't have to move at the call site, which might not even be possible. Only worry about adding overloads for both if you have evidence that there's any measurable issue with performance. – underscore_d Jun 02 '20 at 18:21
  • 1
    @jvd What is it explicit about, though? AFAIK, the only case in which one should only provide a move overload is if the receiving object takes unique ownership of the resource. There's no evidence that is the case as the OP only mentioned performance concerns. So only providing that just hinders users for no apparent reason. **Edit**: and, as HTNW points out, doesn't even work because you don't move inside the implementation anyway! – underscore_d Jun 02 '20 at 18:22
  • 1
    Does this answer your question? [Is the pass-by-value-and-then-move construct a bad idiom?](https://stackoverflow.com/questions/21035417/is-the-pass-by-value-and-then-move-construct-a-bad-idiom) – underscore_d Jun 02 '20 at 18:22
  • 2
    Note that the second one copies. The "explicit" rvalue-reference is *confusing*, because `std::vector &&Inner_` means that `Inner_` *binds to* rvalues, but `Inner_` as an expression is still an lvalue, which will be copied from unless you `std::move` it again. – HTNW Jun 02 '20 at 18:23
  • @HTNW So they're not functionally the same then? Is the first one a better choice then if the goal is to to optimize performance? – Oscar Jun 02 '20 at 18:24
  • 1
    @Oscar You can make them perform the same by changing to `Inner(std::move(Inner_))` in the initializer list. At that point, this question probably becomes a duplicate of the linked question, and I would prefer the first version for simplicity. – HTNW Jun 02 '20 at 18:25
  • If the goal is to optimise performance, then measure. But first, know how to actually write a move constructor. And finally, don't write one unless you know you need to. There are almost always better things to spend your time worrying about! – underscore_d Jun 02 '20 at 18:26
  • @HTNW So you mean you would invoke the std::move keyword twice? Once in main and once in the constructor? Is it just a way of being explicit in what you're doing then or what's the difference? – Oscar Jun 02 '20 at 18:27
  • @underscore_d I was worrying about different things (and asking questions about them here), but people couldn't help themselves from pointing out how me passing the vector by reference was bad practice and telling me to use std::move instead so here I am lol. And I suppose the goal isn't actually to optimize performance as it is to learn best practice (when it comes to optimizing performance), in this case it doesn't make a measurable difference – Oscar Jun 02 '20 at 18:28
  • @underscore_d Passing by value will have the effect that lvalues will be copy-constructed. Explicit moving c-tor would prevent that. That being said, I am not religious about this. If passing by value is expensive, there can be a separate const reference version, or a separate moving version. Depends on the requirements of the code. – jvd Jun 02 '20 at 18:30
  • 2
    @Oscar Yes. The only difference between taking the argument by-rvalue-reference and taking it by-value is *flexibility* (as described in the answer), with the latter being more flexible and therefore generally better. Explicitness isn't really as important. – HTNW Jun 02 '20 at 18:30
  • @underscore_d I see, so really having the constructor take an rvalue just servers to limit my user. They're doing the same thing except in the rvalue case you *have* to use std::move and in method1 you can do either way, right? Thank you both for explaining it to me – Oscar Jun 02 '20 at 18:41

1 Answers1

7
StatisticsCompiler::StatisticsCompiler(std::vector<Wrapper<StatisticsMC>> Inner_) :Inner(std::move(Inner_))

This constructor takes the argument by value. The parameter can either be copy constructed from an lvalue argument, or move constructed from an rvalue. This ability to choose between copying from lvalue and moving from rvalue, combined with the simplicity, is why this approach is recommended.

The member is always moved from that copied or moved argument.

StatisticsCompiler gathererCombiner(gathererArray);

You passed an lvalue; therefore the parameter is copied. You could use the move constructor instead by passing an rvalue:

StatisticsCompiler gathererCombiner(std::move(gathererArray));

Or you could even use a prvalue:

 StatisticsCompiler gathererCombiner({
    meanGatherer,
    absQuantileGatherer,
    relVaRGatherer,
    relESGatherer,
 });

StatisticsCompiler::StatisticsCompiler(std::vector<Wrapper<StatisticsMC>>&& Inner_) :Inner(Inner_)

This constructor accepts only rvalue arguments. This is not as flexible as the first suggestion which also can accept lvalues.

This approach always copies the parameter into the member. You might want to move instead:

Inner(std::move(Inner_))

Conclusion: When you want to store an object given as an argument, then passing by value (the first example) is good default choice. If you don't need the passed argument afterwards, then you can choose to move from it.

eerorika
  • 232,697
  • 12
  • 197
  • 326
  • Yup. Pass-by-value then move-in-implementation means one overload and no restrictions on what the user can/must do with what they're passing. It's kind of unfortunate that this seems like a leakage of implementation details (the choice to move if possible), but that's just how the language is and must be. It's the best way to make 99% of scenarios work Well Enough (R) (TM). Having to add move overloads for every constructor/method/argument just leads to a combinatorial explosion of member function overloads, which is wasted time if it's not proven that the extra copies are a real-world problem – underscore_d Jun 02 '20 at 18:29
  • 1
    It's worth noting that the advice to take by value really only applies when you're going to move from it into your internals. In almost all other cases, taking a reference of some sort is preferred. – Mooing Duck Jun 02 '20 at 18:38