0

Is this a correct approach to pass std::string parameters to the Impl's constructor with move semantics?

#include <string>
#include <memory>

using namespace std;

class Foo
{
    struct Impl;
    unique_ptr<Impl> impl_;
public:
    Foo(string s);
};

struct Foo::Impl
{
    string s_;
    Impl(string s) : s_(std::move(s)) {}
};

Foo::Foo(string s) : impl_(make_unique<Foo::Impl>(move(s)))
{
}

Or should the Impl's ctor be defined as:

Impl(string&& s) : s_(std::move(s)) {}
VladG
  • 105
  • 7
  • 1
    Using an rvalue reference might make the code a little faster as it cuts out a move. As always, try both and measure the results. Depending on the sizes of the strings, and if your implementation using SSO, cutting out a move could make a big difference, or it might not really matter. – NathanOliver Aug 16 '21 at 21:38
  • Do I need std::move() in the Impl's ctor for the rvalue reference case? It compiles either way with or with out the move call. – VladG Aug 16 '21 at 21:42
  • 1
    Yes, you will still need to use `std::move`. Anything with a name is an lvalue, and that includes rvalue references, so if you want the move to happen, you need to convert it back to an rvalue with `std::move`. – NathanOliver Aug 16 '21 at 21:44
  • I see, so without the std::move() s_ will just be copy constructed? – VladG Aug 16 '21 at 21:45
  • 1
    Yep. Without it, you have an lvalue, and those are never moved unless you use `std::move`. There are a couple edge cases to that statement, but those deal with local objects with automatic storage duration and either being returned or thrown out of the local scope. – NathanOliver Aug 16 '21 at 21:46
  • Ok, it clicked now :) – VladG Aug 16 '21 at 21:48
  • Related to [pass-by-value-vs-pass-by-rvalue-reference](https://stackoverflow.com/questions/37935393/pass-by-value-vs-pass-by-rvalue-reference) – Jarod42 Aug 17 '21 at 07:23

1 Answers1

2

Is this a correct approach

Impl(string s) : s_(std::move(s)) {}

This is fine.

Or should the Impl's ctor be defined as:

Impl(string&& s)

This would generally be less useful because you wouldn't be able to pass lvalue strings as arguments. That said, if the Foo::Foo(string s) is the only context where Impl's constructor is called, then that lack of general usefulness wouldn't be a problem in practice and thus this would also be fine.

P.S. Once you actually define Impl in a separate translation unit, you'll find that you need to have user declared special member functions (destructor, move, assignment) of Foo because their definition depends on the definition of Impl.

eerorika
  • 232,697
  • 12
  • 197
  • 326