Re: Implementing swap()

[Jonas Platte <jonasplatte gmail com>]

Am 10.09.2015 um 23:11 schrieb Murray Cumming:
> Now that our Glib::Objects and Gtk::Widgets are movable, so they can be
> placed in standard containers, I guess we should implement swap() so the
> standard containers can use that too. And maybe it's just generally good
> practice to provide a swap() for all classes. Stop me at this point if
> I'm wrong about that.

Sounds like a good idea. I've never had a deep look into the gtkmm
sources, but I'd like to propose a completely different implementation
of all these functions, which if applicable at all, are a bit shorter
and easier. This is all based on the assumption that the classes we are
talking about all only have a single member variable that is a pointer
to the corresponding C structure, owned by these wrapper classes. If
that's not the case, this whole proposal might not make sense.

So, before even talking about the swap implementation, I would make one
big change, which is to exchange these structs that are manually managed
via pointers, new and delete with std::unique_ptr<StructName>. Apart
from making the following function implementations a lot simpler, it
would probably erase the need for an explicit destructor, and the move
constructor and move assignment operator could be `= default`ed.

> If we do want to provide swap() for these classes, I'd like to agree
> about how best to implement them:
>
> We already have swap() for our boxed type classes, such as Gdk::Color.
> We implement a member swap():

I agree, that makes sense.

> void Color::swap(Color& other) noexcept
> {
>   GdkColor *const temp = gobject_;
>   gobject_ = other.gobject_;
>   other.gobject_ = temp;
> }

With gobject_ being a std::unique_ptr<GdkColor> in this case, this could
be simplified to:

void Color::swap(Color& other) noexcept
{
  gobject_.swap(other.gobject_);
}

> and we implement a standalone swap in our namespace, so that std::swap()
> can use it. (See http://www.cplusplus.com/reference/utility/swap/ )
> We implement that standalone swap with our member swap():
>
> inline void swap(Color& lhs, Color& rhs) noexcept
>   { lhs.swap(rhs); }

Checked with cppreference.com, seems like the right way of doing thing
(although it seems a bit weird to me that specializing std::swap isn't
preferred over creating it in your own namespace, never heard about
argument dependent lookup before).

> At the moment we are using the member swap in our move assignment
> operator and copy assignment operators (operator=):
>
> Color& Color::operator=(Color&& other) noexcept
> {
>   Color temp(other);
>   swap(temp);
>   return *this;
> }

This would become:

Color& Color::operator=(Color&& other) noexcept
{
  gobject_ = std::move(other.gobject_);
  return *this;
}

> Color& Color::operator=(const Color& other)
> {
>   Color temp(other);
>   swap(temp);
>   return *this;
> }

This implementation makes sense with or without changing the type of
gobject_ as far as I can tell.

> The member swap() is obviously useful for those operator=()
> implementations, though I guess swap(*this, temp) would work too.

If not changing the type of gobject_, that's what I'd do for the move
assignment operator.

> But the member swap() isn't inline so our standalone swap() isn't as
> efficient as it could be.

I heavily doubt that inline makes any difference in performance once you
enable compiler optimizations. I think it doesn't have any real effect
other than giving developers the ability to define functions in header
files.

> So:
> Should we make our member swap() methods inline?

Not for performance reasons, unless we want the best possible
performance for users of very stupid compilers.

> Should we not have member swap() methods and just use our standalone
> swap() functions?

I don't have a strong opinion on this, a friend function would work as
well. I slightly prefer having member functions, for when you want to
use them in application code.

> And, should we implement swap() using std::swap() on the member
> variables, or even std::move() on the member variables?

Now after reading this question, I'm not sure anymore if my suggestion
really makes no sense without changing the type of gobject_. Anyway, I
don't see why you would do the swap manually, instead of through the
standard library (if through std::swap or std::unique_ptr<T>::swap).