Correct usage of rvalue references as parameters

Let's take the following method as an example:

void Asset::Load( const std::string& path )
{
    // complicated method....
}

General use of this method would be as follows:

Asset exampleAsset;
exampleAsset.Load("image0.png");

Since we know most of the time the Path is a temporary rvalue, does it make sense to add an Rvalue version of this method? And if so, is this a correct implementation;

void Asset::Load( const std::string& path )
{
    // complicated method....
}
void Asset::Load( std::string&& path )
{
     Load(path); // call the above method
}

Is this a correct approach to writing rvalue versions of methods?

For your particular case, the second overload is useless.

With the original code, which has just one overload for Load, this function is called for lvalues and rvalues.

With the new code, the first overload is called for lvalues and the second is called for rvalues. However, the second overload calls the first one. At the end, the effect of calling one or the other implies that the same operation (whatever the first overload does) will be performed.

Therefore, the effects of the original code and the new code are the same but the first code is just simpler.

Deciding whether a function must take an argument by value, lvalue reference or rvalue reference depends very much on what it does. You should provide an overload taking rvalue references when you want to move the passed argument. There are several good references on move semantincs out there, so I won't cover it here.

Bonus:

To help me make my point consider this simple probe class:

struct probe {
	probe(const char*  ) { std::cout << "ctr " << std::endl; }
	probe(const probe& ) { std::cout << "copy" << std::endl; }
	probe(probe&&      ) { std::cout << "move" << std::endl; }
};

Now consider this function:

void f(const probe& p) {
	probe q(p);
	// use q;
}

Calling f("foo"); produces the following output:

ctr
copy

No surprises here: we create a temporary probe passing the const char* "foo". Hence the first output line. Then, this temporary is bound to p and a copy q of p is created inside f. Hence the second output line.

Now, consider taking p by value, that is, change f to:

void f(probe p) {
	// use p;
}

The output of f("foo"); is now

ctr

Some will be surprised that in this case: there's no copy! In general, if you take an argument by reference and copy it inside your function, then it's better to take the argument by value. In this case, instead of creating a temporary and copying it, the compiler can construct the argument (p in this case) direct from the input ("foo"). For more information, see Want Speed? Pass by Value. by Dave Abrahams.

There are two notable exceptions to this guideline: constructors and assignment operators.

Consider this class:

struct foo {
	probe p;
	foo(const probe& q) : p(q) { }
};

The constructor takes a probe by const reference and then copy it to p. In this case, following the guideline above doesn't bring any performance improvement and probe's copy constructor will be called anyway. However, taking q by value might create an overload resolution issue similar to the one with assignment operator that I shall cover now.

Suppose that our class probe has a non-throwing swap method. Then the suggested implementation of its assignment operator (thinking in C++03 terms for the time being) is

probe& operator =(const probe& other) {
	probe tmp(other);
	swap(tmp);
	return *this;
}

Then, according to the guideline above, it's better to write it like this

probe& operator =(probe tmp) {
	swap(tmp);
	return *this;
}

Now enter C++11 with rvalue references and move semantics. You decided to add a move assignment operator:

probe& operator =(probe&&);

Now calling the assignment operator on a temporary creates an ambiguity because both overloads are viable and none is preferred over the other. To resolve this issue, use the original implementation of the assignment operator (taking the argument by const reference).

Actually, this issue is not particular to constructors and assignment operators and might happen with any function. (It's more likely that you will experience it with constructors and assignment operators though.) For instance, calling g("foo"); when g has the following two overloads raises the ambiguity:

void g(probe);
void g(probe&&);

Unless you're doing something other than calling the lvalue reference version of Load, you don't need the second function, as an rvalue will bind to a const lvalue reference.

> Since we know most of the time the Path is a temporary rvalue, does it make sense to add an Rvalue version of this method?

Probably not... Unless you need to do something tricky inside Load() that requires a non-const parameter. For example, maybe you want to std::move(Path) into another thread. In that case it might make sense to use move semantics.

> Is this a correct approach to writing rvalue versions of methods?

No, you should do it the other way around:

void Asset::load( const std::string& path )
{
     auto path_copy = path;
     load(std::move(path_copy)); // call the below method
}
void Asset::load( std::string&& path )
{
    // complicated method....
}

It's generally a question of whether internally you will make a copy (explicitly, or implicitly) of the incoming object (provide T&& argument), or you will just use it (stick to [const] T&).

If your Load member function doesn't assign from the incoming string, you should simply provide void Asset::Load(const std::string& Path).

If you do assign from the incoming path, say to a member variable, then there's a scenario where it could be slightly more efficient to provide void Asset::Load(std::string&& Path) too, but you'd need a different implementation that assigns ala loaded_from_path_ = std::move(Path);.

The potential benefit is to the caller, in that with the && version they might receive the free-store region that had been owned by the member variable, avoiding a pessimistic delete[]ion of that buffer inside void Asset::Load(const std::string& Path) and possible re-allocation next time the caller's string is assigned to (assuming the buffer's large enough to fit its next value too).

In your stated scenario, you're usually passing in string literals; such caller's will get no benefit from any && overload as there's no caller-owned std::string instance to receive the existing data member's buffer.

Here's what I do when trying to decide on the function signature

1. (const std::string& const_lvalue) argument is read only
2. (std::string& lvalue) I can modify argument (usually put something in) so the change would be VISIBLE to the caller
3. (std::string&& rvalue) I can modify argument (usually steal something from), zero consequences since the caller would no longer see/use this argument (consider it self destroyed after function returns)

All three of them are "pass-by-reference", but they show different intentions. 2+3 are similar, they can both modify the argument but 2 wants the modification to be seen by the caller whereas 3 doesn't.

// (2) caller sees the change argument
void ModifyInPlace(Foo& lvalue){
  delete lvalue.data_pointer;
  lvalue.data_pointer = nullptr; 
}

// (3) move constructor, caller ignores the change to the argument
Foo(Foo&& rvalue)
{
  this->data_pointer = that.data_pointer;
  that.data_pointer = nullptr;
}