Den 2015-09-11 kl. 00:11, skrev Jonas
Platte:
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).
