Is it worth adding a move-enabled setter?

Question

This post rambles a bit so before I get into it I want to be clear what I'm asking: Have you added move-enabled setters to your code and have you found it's worth the effort? And how much of the behaviour I've found can I expect might be compiler-specific?

What I'm looking at here is whether it's worth while to add a move-enabled setter function in cases where I'm setting a property of a complex type. Here I have move-enabled Bar and Foo which has a Bar property which may be set.

class Bar {
public:
    Bar() : _array(1000) {}
    Bar(Bar const & other) : _array(other._array) {}
    Bar(Bar && other) : _array(std::move(other._array)) {}
    Bar & operator=(Bar const & other) {
        _array = other._array;
        return *this;
    }
    Bar & operator=(Bar && other) {
        _array = std::move(other._array);
        return *this;
    }
private:
    vector<string> _array;
};

class Foo {
public:
    void SetBarByCopy(Bar value) {
        _bar = value;
    }
    void SetBarByMovedCopy(Bar value) {
        _bar = std::move(value);
    }
    void SetBarByConstRef(Bar const & value) {
        _bar = value;
    }
    void SetBarByMove(Bar && value) {
        _bar = std::move(value);
    }
private:
    Bar _bar;
};

Generally speaking in the past I've gone with a const-ref for setter functions for non built-in types. The options I looked at were to pass by-value then move (SetByMovedCopy), pass by const-ref then copy (SetByConstRef) and finally to accept by r-value-ref then move (SetByMove). As a baseline I also included pass-by-value then copy (SetByCopy). FWIW, the compiler complained of ambiguity if including both pass-by-value and r-value-ref overloads.

In experiments with the VS2010 compiler, this is what I've found:

Foo foo;
Bar bar_one;

foo.SetByCopy(bar_one);
// Bar::copy ctor called (to construct "value" from bar_one)
// Foo::SetByCopy entered
// Bar::copy operator= called (to copy "value" to _bar)
// Foo::SetByCopy exiting
// Bar::dtor called (on "value")

value is copy-constructed from bar_one, then value is copied to bar. value is destructed and incurs any costs of destructing a complete object. 2 copy operations are executed.

foo.SetByMovedCopy(bar_one);
// Bar::copy ctor called (to construct "value" from bar_one)
// Foo::SetByCopy entered
// Bar::move operator= called (to move "value" into _bar)
// Foo::SetByCopy exiting
// Bar::dtor called (to destruct the moved "value")

value is copy-constructed from bar_one, then value is moved into _bar, then the gutted value is destructed after the function exits, presumably at lower cost. 1 copy and 1 move operation.

foo.SetByConstRef(bar_one);
// Foo::SetByConstRef entered
// Bar::copy operator= called (to copy bar_one into _bar)
// Foo::SetByConstRef exiting

bar_one is copied directly into _bar. 1 copy operation.

foo.SetByMove(std::move(bar_one))
// Foo::SetByMove entered
// Bar::move operator= called (to move "value" into _bar)
// Foo::SetByMove exited

bar_one is moved directly into _bar. 1 move operation.

---

So the const-ref and move versions are the most efficient in this case. Now, more to the point, what I'm looking to do is something like this:

void SetBar(Bar const & value) { _bar = value; }
void SetBar(Bar && value) { _bar = std::move(value); }

What I've found happens here is that if you call Foo::SetBar, the compiler picks the function based on whether you're passing an l-value or an r-value. You can force the issue by calling std::move as such:

foo.SetBar(bar_one); // Const-ref version called
foo.SetBar(Bar()); // Move version called
foo.SetBar(std::move(bar_one)); // Move version called

I shudder to think of adding all these move setters, but I think it might result in a pretty significant performance gain in cases where a temporary is passed to the SetBar function and going forward I can gain even more by applying std::move where appropriate.

Answer 1

Another option would be a template:

template <typename T>
typename std::enable_if<std::is_assignable<Foo, T>::value>::type set(T && t)
{
    foo_ = std::forward<T>(t);
}

That way you can match anything that's convertible, and any value category. Don't forget to #include <type_traits> to get is_assignable. (You shouldn't omit the enable_if so that your function doesn't erroneously show up in other trait checks.)

Answer 2

tl;dr: Use PassByValue. In your PassByValue, assign via std::move. Use std::move whenever it makes sense to do so when calling the setter (i.e. foo.PassByValue(std::move(my_local_var))) if you know that the setter is also using it.

That single version of the setter, taking the object by-value, handles the most common uses in an efficient way, allows the compiler to do the optimization, is cleaner and more readable.

---

I like the answers that were given but I think the best answer to my question came from the comments in the original question which lead me to approach how I was testing these methods from a different angle, so I'm going to be that kind of guy provide the answer my own question.

class Foo {
public:
    void PassByValue(vector<string> value) {
        _bar = std::move(value);
    }
    void PassByConstRefOrMove(vector<string> const & value) {
        _bar = value;
    }
    void PassByConstRefOrMove(vector<string> && value) {
        _bar = std::move(value);
    }
    void Reset() {
        std::swap(_bar, vector<string>());
    }
private:
    vector<string> _bar;
};

To test, I compared 3 situations: Passing an l-value, passing an r-value, and passing an explicitly-moved l-value as an r-value reference.

The purpose of this test was not to measure the overhead of the function calls. That's in the realm of micro-optimization. What I'm trying to do is dissect compiler behaviour and work out a best-practice for implementing and using setter functions.

vector<string> lots_of_strings(1000000, "test string");
Foo foo;
// Passing an l-value
foo.PassByValue(lots_of_strings);
// Passing an r-value
foo.PassByValue(vector<string>(1000000, "test string"));
// Passing an r-value reference
foo.PassByValue(std::move(lots_of_strings));

// Reset vector because of move
lots_of_strings = vector<string>(1000000, "test string");
// l-value, calls const-ref overload
foo.PassByConstRefOrMove(lots_of_strings);
// r-value, calls r-value-ref overload
foo.PassByConstRefOrMove(vector<string>(1000000, "test string"));
// explicit move on l-value, calls r-value-ref overload
foo.PassByConstRefOrMove(std::move(lots_of_strings));

Excluded for brevity is that I also called Foo::Reset() after every call to clean out _bar. The results (after 1000 passes):

PassByValue:
  On l-value    : 34.0975±0.0371 ms
  On r-value    : 30.8749±0.0298 ms
  On r-value ref:  4.2927e-3±4.2796e-5 ms

PassByConstRefOrMove:
  On l-value    : 33.864±0.0289 ms
  On r-value    : 30.8741±0.0298 ms
  On r-value ref:  4.1233e-3±4.5498e-5 ms

Resetting foo after every call is perhaps not a perfect emulation of real life. When I didn't do that and instead set up _bar to already have some data in place, PassByConstRef performed much better on the l-value test and a little better on the r-value test. I believe it performed much better on the l-value test because vector realised it didn't need to re-allocate and just copied the contents straight over. In the case of moves though, it would de-allocate regardless, and incur that cost. But this is vector-specific behaviour and I'm not sure if it should count for much in this context.

Otherwise the results were similar. The error margin listed is just based on the standard error of the results, and does not consider the accuracy of the CPU timer I used.

The conclusion I'd draw is it's better to just pass by value. For this contrived scenario, the two methods were almost identical in terms of performance and certainly good enough for government work, but the ease of implementation and clarity of the interface using pass-by-value gives it the edge in my books. I'll just have to remember to use std::move when calling a setter when it makes sense to do so as it can make a significant difference.

Tip of the hat to @Luc_Danton for pointing me in this direction.

Answer 3

One technique I've used is to use macros to automatically generate the getters/setters for class attributes. Aside from less boilerplate code, this has other advantages, such as using the macro to automatically provide typedefs and to enforce consistent interface semantics. I don't find it to be any more difficult to read than other code, either.

For example,

#define ATTRIBUTE(type, name) \
public: \
typedef type name##_type; \
\
void set_##name( type const &p_##name ) { \
  m_##name = p_#name; \
} \
\
void set_##name( type &&p_##name ) { \
  m_##name = std::move( p_##name ); \
\
type const &get_##name( ) const { \
  return m_##name; \
} \
\
type *mutable_##name( ) { \
  return &m_##name; \
} \
\
private: \
\
type m_##name;

Now your code just looks like this:

struct blah {
  ATTRIBUTE( std::string, foo );
};

I actually think that's easier to read than a bunch of setters and getters. (There's a good reason to include the mutable accessor: it means that you don't have to create complete copies and can instead modify the member in place, and it's more explicit than a non-const getter.) Where this gets a bit hairy is when you use templates as macro arguments since the preprocessor will split at commas, but you can overcome this by defining a COMMA macro:

#define COMMA ,

struct blah {
  ATTRIBUTE( std::map< foo COMMA bar >, baz );
};

Answer 4

I believe overloading a setter to take lvalue and rvalue references still makes sense. The way tests were performed by the author assume that the setter is called on a fresh instance of the class, which may not always be the case.

Typically there is only one parameter that is passed to a setter, so the number of overloads will not become combinatorial. And regarding code duplication, usually setters are simple methods, so there will be not much code to duplicate. Otherwise, you may define a helper template method (or a free function) taking in a universal reference:

void Foo::set(const Bar& bar)
{
    setImpl(bar);
}

void Foo::set(Bar&& bar)
{
    setImpl(std::move(bar));
}

template<typename T>
void Foo::setImpl(T&& bar)
{
    // Some complex code ...

    this->bar = std::forward<T>(bar);

    // More complex code ...
}

I suggest adding such template method as implementation detail rather than a public method for the following reasons:

1. If accessible by the client code, it must be properly conditioned such that it does not accept values of inappropriate types.
2. Template methods cannot be virtual.
3. Template magic is not so readable.

Regarding passing by value, I see the following downsides:

1. When a value needs to be copied, it will always have to allocate new memory. In contrast, taking an argument in by reference and assigning it to the internal variable could reuse existing capacity.

    void Foo::set(const Bar& bar) 
    {
        // Memory for this->bar may already be allocated, reuse it
        this->bar = bar;
    }

2. Setters taking in an rvalue reference can often be declated noexcept. Although technically exceptions thrown while passing an argument don't violate the noexcept guarantee, it may be counter-intuitive for users.

    void Foo::set(Bar&& bar) noexcept
    {
        static_assert(std::is_nothrow_move_assignable_v<Bar>);
        this->bar = std::move(bar);
    }

    /*
    // Technically also works but calling a noexcept function which potentially 
    // may throw an exception while making a copy of an argument can be confusing
    void Foo::set(Bar bar) noexcept
    {       
        this->bar = std::move(bar);
    } */