universal reference c++11 code duplication

Question

In my project I have function like this:

bool VectorList::put(const Pair &p);

This adds the Pair into the VectorList by copying the Pair.

I can use it like this:

Pair p { "key", "value" };

VectorList v;
v.put(p);

// or
v.put(Pair{ "anotherkey", "anothervalue" });

However in second case an unnecessary object is created, so I want to do

bool VectorList::put(Pair &&p);

I checked how this is done in vector (gcc, llvm) and there is 100% same code in both methods, except equal / std::move() line.

Is there some way I could do it without code duplication?

---

put() looks similar to this:

struct Node{
    Pair pair;
    AdditionalThings at;
};

bool VectorList::put(const Pair &p){
    if (not_good_for_insert(p))
         return false;
    // ...
    Node node = create_node();
    node.pair = p;
    // ...
    return true;
}

Answer 1

Yes, use perfect forwarding:

template <typename P>
bool VectorList::put (P &&p) {
    //can't forward p here as it could move p and we need it later
    if (not_good_for_insert(p)) 
     return false;
    // ...
    Node node = create_node();
    node.pair = std::forward<P>(p);
    // ...
    return true;
}

Another possibility is to just pass by value like in Maxim's answer. The advantage of the perfect-forwarding version is that it requires no intermediate conversions if you pass in compatible arguments and performs better if moves are expensive. The disadvantage is that forwarding reference functions are very greedy, so other overloads might not act how you want.

Note that Pair &&p is not a universal reference, it's just an rvalue reference. Universal (or forwarding) references require an rvalue in a deduced context, like template arguments.

Answer 2

The ideal solution is to accept a universal reference, as TartanLlama advises.

The ideal solution works if you can afford having the function definition in the header file. If your function definition cannot be exposed in the header (e.g. you employ Pimpl idiom or interface-based design, or the function resides in a shared library), the second best option is to accept by value. This way the caller can choose how to construct the argument (copy, move, uniform initialization). The callee will have to pay the price of one move though. E.g. bool VectorList::put(Pair p);:

VectorList v;
Pair p { "key", "value" };
v.put(p); 
v.put(std::move(p));
v.put(Pair{ "anotherkey", "anothervalue" });
v.put({ "anotherkey", "anothervalue" });

And in the implementation you move from the argument:

bool VectorList::put(Pair p) { container_.push_back(std::move(p)); }

---

Another comment is that you may like to stick with standard C++ names for container operations, like push_back/push_front, so that it is clear what it does. put is obscure and requires readers of your code to look into the source code or documentation to understand what is going on.

Answer 3

With help of TartanLlama, I made following test code:

#include <utility>
#include <iostream>
#include <string>

class MyClass{
public:
    MyClass(int s2) : s(s2){
        std::cout << "c-tor " << s << std::endl;
    }

    MyClass(MyClass &&other) : s(other.s){
        other.s = -1;

        std::cout << "move c-tor " << s << std::endl;
    }

    MyClass(const MyClass &other) : s(other.s){
        std::cout << "copy c-tor " << s << std::endl;
    }

    ~MyClass(){
        std::cout << "d-tor " << s << std::endl;
    }

public:
    int s;
};

// ==============================

template <typename T>
MyClass process(T &&p){
    MyClass out = std::forward<T>(p);

    return out;
}

// ==============================

void t1(){
    MyClass out = process( 100 );
}

void t2(){
    MyClass out = process( MyClass(100) );
}

void t3(){
    MyClass in  = 100;
    MyClass out = process(std::move(in));

    std::cout <<  in.s << std::endl;
    std::cout << out.s << std::endl;
}

void t4(){
    MyClass in  = 100;
    MyClass out = process(in);

    std::cout <<  in.s << std::endl;
    std::cout << out.s << std::endl;
}


int main(int argc, char** argv){
    std::cout << "testing fast c-tor"   << std::endl;   t1();   std::cout << std::endl;
    std::cout << "testing c-tor"        << std::endl;   t2();   std::cout << std::endl;
    std::cout << "testing move object"  << std::endl;   t3();   std::cout << std::endl;
    std::cout << "testing normal object"    << std::endl;   t4();   std::cout << std::endl;
}

Output on gcc is following:

testing fast c-tor
c-tor 100
d-tor 100

testing c-tor
c-tor 100
move c-tor 100
d-tor -1
d-tor 100

testing move object
c-tor 100
move c-tor 100
-1
100
d-tor 100
d-tor -1

testing normal object
c-tor 100
copy c-tor 100
100
100
d-tor 100
d-tor 100