However, I really want behaviour to be logical, and I don't think that swapping internal state for resources like windows during move operations is logical.
A classical implementation of Copy-and-Swap would be like this:
class Image
{
Image();
Image(const Image& copied);
Image(Image&& moved);
void swap(Image& other);
Image& operator= (const Image& copied) {
Image tmp(copied);
tmp.swap(*this);
return *this;
} // object 'tmp' (with previous contents of *this) is destroyed
Image& operator= (Image&& moved)
{
moved.swap(*this);
return *this;
} // object 'moved' (with previous contents of *this) is destroyed
};
Since the temporary object is destroyed after a copy operation, it does not matter what it contains. A moved-from object is typically destroyed after the move as well, but there are exceptions like std::unique_ptr which define a behavior. So, unless SFML would explicitly guarantee that a moved-from image is equal to an empty image, we can put any valid content into moved-from objects. Otherwise we would simply involve a default-constructed object to "reset" the image after moving.
The reason why this idiom exists in the first place, is because the naive (or compiler-generated) implementation does this:
Image& operator= (const Image& copied)
{
member0 = copied.member0;
member1 = copied.member1;
// ...
memberN = copied.memberN;
return *this;
}
Image& operator= (Image&& moved)
{
member0 = std::move(moved.member0);
member1 = std::move(moved.member1);
// ...
memberN = std::move(moved.memberN);
return *this;
}
Now imagine that during "...", an exception occurs while copying/moving. The assignment operation is aborted, but both objects are in an inconsistent, "half-moved" or "half-copied" state. This is mostly a problem for copies, as moves should be designed to not fail (however, this is not always possible, e.g. when moves fall back to copies, for raw arrays for example).
What Copy-and-Swap does is -- instead of overwriting the object in-place -- first creating a new valid object, swapping with that and then destroying the old one. Since both swap and destroy operations should not fail, only the construction of the new object can fail, in which case the object is left in the previous, valid state.
So much about the theory. What I mentioned above is absolutely crucial when designing generic building blocks like containers, smart pointers, graphs or other data structures. You don't know what the user will put inside, so you have to assume copies can fail, and design the code accordingly.
Now, in practice, you often have better knowledge about what types are involved, and can exploit this knowledge to simplify code. If SFML resorts to not throwing exceptions during copy/move operations, and has no APIs where user-defined types could trigger exceptions, we can avoid the extra effort and try to use the compiler-generated methods where possible.
Pragmatically, I would attempt to use default move (and even copy) constructors/assignment operators wherever possible -- it's just so much easier to reason about code, to add new fields, etc. But since SFML operates with resources in several places, it involves custom ownership semantics here and there. These are the cases we need to analyze and determine what is the greatest common denominator.
A note worth mentioning is that if the internal handles were unique pointers then default move constructors would be perfect. Maybe this is a more valid approach for move semantics in some cases.
In sf::Shader, the private members are those:
////////////////////////////////////////////////////////////
// Types
////////////////////////////////////////////////////////////
typedef std::map<int, const Texture*> TextureTable;
typedef std::map<std::string, int> UniformTable;
////////////////////////////////////////////////////////////
// Member data
////////////////////////////////////////////////////////////
unsigned int m_shaderProgram; //!< OpenGL identifier for the program
int m_currentTexture; //!< Location of the current texture in the shader
TextureTable m_textures; //!< Texture variables in the shader, mapped to their location
UniformTable m_uniforms; //!< Parameters location cache
The two std::maps would move fine (emptying the source), but the integers will be copied. So, the default move implementation would leave a half-working instance behind.
As mentioned, in SFML we need to decide whether we give guarantees on moved-from objects or not.
For the sake of least surprise and simplicity, it might be more user-friendly to make moved-from objects behave like default-constructed ones.
In that regard, your current implementation is inconsistent:
sf::Shader a = ...;
sf::Shader b = std::move(a); // a is now empty
however:sf::Shader a = ...;
sf::Shader b = ...;
b = std::move(a) // a now has contents of b
Since you already implement Move-and-Swap (but backwards), you might as well reuse swap() to always empty the source. In general, it's better to call the copy/move constructor in the copy/assignment operator, not vice versa.
A generic approach is this. It does a bit more than necessary, but this pattern can be applied everywhere, and the advantage is that the actual logic is in the swap() and only there --- move ctor/op= don't need to be changed when the fields change.
class Shader
{
Shader();
Shader(Shader&& moved)
: Shader()
{
moved.swap(*this); // empties source
}
Shader& operator= (Shader&& moved)
{
Shader tmp(std::move(moved)); // invoke move ctor, empties 'moved'
tmp.swap(*this);
return *this;
} // destroy 'tmp',
void swap(Shader& other); // actual logic is here
}
We should also establish a naming convention for parameter names. For copy constructors, SFML currently uses copy. This is slightly misleading, as the parameter is the object being copied (origin) and not the copy (replica). In Thor I used origin and source, but I think we can be more precise.
What about copied and moved?
This makes it immediately clear that you're dealing with a copy or move ctor/op=.
[Edit] I made an example how I would probably implement it here:
https://github.com/SFML/SFML/tree/feature/move-semantics
One commit with unified copy/move op=, the whole branch diff with separate ones.
Not tested -- those things might be a candidate for unit tests.
If I may introduce my point and suggestions.
First of all, I agree with copy-and-swap in advance.
Second of all, C++ programmers often make the mistake of supplying classes with too much noise (defining empty destructors, implementing default copy/move constructors, and default assignment operator overloads) which does more harm than good for readability and maintainability.
The rule of thumb should be to default to the Rule of Zero. Only when it does not suffice, fallback to the Rule of Three or Five (preferably) starting with compiler defaults ("= default") or Four-and-a-Half even. It is most intuitive when the noise is removed. An empty non-virtual destructor makes the class non-trivially destructible from the Standard's point of view. A non-noexcept move constructor makes the class non-no-throw-move-constructible.
The question could come: why be strict about the noexcept specifier in move semantics? Because STL utilizes this mechanic AND most importantly it assures that the move construction/assignment is intuitive. If a class can not afford a noexcept move constructor/assignment then it is unable to simply/intuitively transfer its states thus should not use move semantics but a transfer mechanism of some sort. A move constructor/assignment should only do simple transfers. No heap allocations, no checks, no side-effects. Only then it is intuitive.
Should a class need a "move semantic" function with a possible exception - one could write a member function for that purpose and not utilize move constructors/assignment operators.
// Ways of transferring object states in std::unique_ptr
auto ptr = std::make_unique<int>(7);
// a) "Move semantic" transfer
auto otherPtr = std::move(ptr);
// b) A transfer mechanism which does not need to impose noexcept
auto anotherPtr = std::unique_ptr<int>(ptr.release());
Of course, this is an example of std::unique_ptr which has noexcept `release()` function. However, this is just to show how it can be accomplished/worked around to keep a strong exception guarantee on move semantics.