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).
_______________________________________________
gtkmm-list mailing list
gtkmm-list@gnome.org
https://mail.gnome.org/mailman/listinfo/gtkmm-list
