Murray Cumming <murrayc murrayc com> writes:
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.
Correct.
We already have swap() for our boxed type classes, such as Gdk::Color.
We implement a member swap():
void Color::swap(Color& other) noexcept
{
GdkColor *const temp = gobject_;
gobject_ = other.gobject_;
other.gobject_ = temp;
}
For the sake of readability, can be simplified by using std::swap for these pointers. Then it's just a one liner.
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;
}
Color& Color::operator=(const Color& other)
{
Color temp(other);
swap(temp);
return *this;
}
It is usually better, albeit slightly harder to read, to pass in this
case the object by value and let the compiler create the temporary.
Color& Color::operator=(const Color other)
{
swap(other);
return *this;
}
This sometimes produces slightly faster code (saves one indirection),
and it's shorter. Also note, if the member swap is noexcept, also this
version should be.
The member swap() is obviously useful for those operator=() implementations, though I guess swap(*this, temp) would work too. But the member swap() isn't inline so our standalone swap() isn't as efficient as it could be. So: Should we make our member swap() methods inline?
One problem of course, is when you want to provide swap() as a virtual function, because the class is part of a hyerarchy and it needs to call its superclass swap() method. Then it cannot be inlined anyway. I'd say the compiler is smart enough to inline what it needs to without us telling it explicitly what to do.
Should we not have member swap() methods and just use our standalone swap() functions?
No, if we don't want them all be friends of the class in question (admitted that they need access to private fields).
And, should we implement swap() using std::swap() on the member variables, or even std::move() on the member variables?
It depends. If everything is noexcept, I'd say it's quite the same. If
something can throw an exception, it needs to be done by e.g. swapping
tuples, so that it is guaranteed that the object is not left in an
inconsistent state. For instance, this is dangerous [1]:
void C::swap(C& that)
{
using std::swap; // needed to allow Koenig lookup rules to apply
swap(this->a, that.a);
swap(this->b, that.b); // throws
swap(this->c, that.c);
}
Then when catching the exception, "this" and "that" will have had their
first member swapped, but not the other two fields.
In this case, I would write a small templated monad that does a
try/catch at each computation step, and rolls back changes in case of a
throw (and then rethrows). It's not hard to do, and makes the code
much more robust (although with a bit of lambdas in the way). It would
be equivalent to the verbose version:
void C::swap(C& that)
{
using std::swap;
std::swap(a, that.a);
try
{
std::swap(b, that.b);
try
{
swap(c, that.c);
}
catch(...)
{
swap(b, that.b);
throw;
}
}
catch(...)
{
swap(a, that.a);
throw;
}
}
This is not perfect, as there is no guaranteed that the swap will not be
asymmetric and throw while rolling back, but it's better than the first
version. More complex solutions are possible.
Cheers,
Matteo
[1] Full example showing the problem:
------------ %< ----------------- %< ---------------------
#include <exception>
#include <iostream>
#include <tuple>
#include <utility>
namespace ns
{
class Throwing
{
public:
Throwing() = default;
Throwing(const Throwing&)
{
throw std::runtime_error("");
}
};
class Swapper
{
public:
int a;
Throwing b;
int c;
void swap(Swapper& that)
{
using std::swap;
auto t1 = std::tie(a, b, c);
auto t2 = std::tie(that.a, that.b, that.c);
swap(t1, t2);
}
};
} // ~ namespace ns;
namespace std
{
template<>
void swap(::ns::Swapper& a, ::ns::Swapper& b) noexcept(noexcept(a.swap(b)))
{
a.swap(b);
}
}
int
main()
{
ns::Swapper a, b;
a.a = a.c = 1;
b.a = b.c = 2;
try
{
std::swap(a, b);
}
catch(std::runtime_error&)
{
std::cout << "(" << a.a << ", " << a.c << ") vs. "
<< "(" << b.a << ", " << b.c << ")" << std::endl;
}
}
------------ %< ----------------- %< ---------------------
Attachment:
signature.asc
Description: PGP signature