[Development] Setters: Passing by value or const reference?

[Marc Mutz]

Hi Olivier,

On Wednesday April 25 2012, Olivier Goffart wrote:
> In Qt, we traditionally uses const reference as parameter for all our
> setters. This is indeed probably the most efficient thing to do for non-POD
> in C++98 since it avoids copy.  But the situation is different in C++11
[...]
> We could implement a move setter (Foo::setText(QString&&)).  But that would
> mean  duplication, and it could not be inline (access to d_ptr) so binary
> incompatibility between Qt compiled with or without C++11.

The rvalue overloads could only be used by a C++11 compiler, and it would be 
natural for a C++11 app to require a C++11-compiled Qt. There's no such thing 
as binary compatibility between compilations using different flags anyway. 
Indeed, most move constructors (excepting the few you implemented) cannot be 
implemented inline (because they require the definition of ~FooPrivate), so 
you already have an C++11/98 BiC issue once you've implemented all the shared 
classes' move constructors (I did, does anyone else work on this, btw?).

Also, as you noted yourself, it's probably too late to change all setters. 
However, an rvalue reference overload can be added in a BC way, in 5.1.

Taken together, this to me suggests to use rvalue overloads instead of 
pass-by-value. The resulting code duplication can and should be factored in 
the implementation, of course.

> The solution is actually much simpler:
>
> void Foo::setText(QString text) {
> 	d_ptr->text = std::move(text);  // no copy
> }

Most setters look not like the one you quoted, but like this:

void Foo::setText(const QString &text) {
    if ( text == d->text ) return;
    d->text = text; // copy only if needed
    // do something expensive, like repaint, or emit a signal
}

Here, a copy is taken only if you don't early out. When you overload setText() 
for const-reference and rvalue-reference instead, you only incur a copy iff 
it's needed. Not sure that's a problem. Also not sure if slicing could become 
a problem in a pass-by-value world, but const-& and && are both real 
references, so they don't slice.

> foo->setText(tr("hello world")); // no copy here, this is a move.
>
> Now you don't have any copy: you saved two atomic operations (increment,
> decrement) and generated less code.
> If you pass something that is not a temporary, you still have only one
> copy, but on the caller.
>
>  ჻
>
> You notice the use of std::move,  which we can use only in C++11.
> Hence the introduction of a qMove macro[1]
> https://codereview.qt-project.org/24444
> Which would lead to that pattern:
>
> void Foo::setText(QString text) {	d_ptr->text = qMove(text); }

You should use d_ptr->text.swap(text) instead, that's just as fast (maybe even 
faster) and requires no C++11 :)

Talking of swap: I have a half-finished patch that makes Q_DECLARE_SHARED 
implement the two op='s with (copy)-swap (and adds swap() to classes that 
would benefit from it, but don't have it yet).

Foo &Foo::operator=(const Foo &other)
{ if ( this != &other ) { Foo copy(other); swap(other); } return *this; }
Foo &Foo::operator=(Foo &&other)
{ other.swap(*this); return *this; }

I'll try to get it into a reviewable state.

Thanks,
Marc

-- 
Marc Mutz <marc.mutz at kdab.com> | Senior Software Engineer
KDAB (Deutschland) GmbH & Co.KG, a KDAB Group Company
www.kdab.com || Germany +49-30-521325470 || Sweden (HQ) +46-563-540090
KDAB - Qt Experts - Platform-Independent Software Solutions