Welcome, Guest. Please login or register. Did you miss your activation email?

Author Topic: SFML Move Semantics  (Read 36387 times)

0 Members and 1 Guest are viewing this topic.

Nexus

  • Moderator
  • Hero Member
  • *****
  • Posts: 6287
  • Thor Developer
    • View Profile
    • Bromeon
Re: SFML Move Semantics
« Reply #15 on: July 05, 2020, 12:46:57 pm »
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.
Zloxx II: action platformer
Thor Library: particle systems, animations, dot products, ...
SFML Game Development:

Nexus

  • Moderator
  • Hero Member
  • *****
  • Posts: 6287
  • Thor Developer
    • View Profile
    • Bromeon
Re: SFML Move Semantics
« Reply #16 on: July 05, 2020, 01:25:15 pm »
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.
« Last Edit: July 05, 2020, 04:07:33 pm by Nexus »
Zloxx II: action platformer
Thor Library: particle systems, animations, dot products, ...
SFML Game Development:

unlight

  • Newbie
  • *
  • Posts: 33
    • View Profile
    • Email
Re: SFML Move Semantics
« Reply #17 on: July 07, 2020, 02:21:27 am »
Thank you for the theory Nexus, I have not been exposed to the copy-and-swap pattern before so it is interesting to see why and when it could be useful.

After reading through your response and seeing your example implementation, how does this sound as the implementation strategy going forward?

  • Where possible, rely on default implementations of the big 5.
  • For resources that have custom ownership semantics:
  •     Implement move/copy operations via swap method.
  •     Use consistent naming (copied, moved, swapped) for the "rhs" parameter.

Considering you have now created the move semantics branch, should we consider your texture implementation as the first official commit and PRs be accepted when ready to this branch?

Nexus

  • Moderator
  • Hero Member
  • *****
  • Posts: 6287
  • Thor Developer
    • View Profile
    • Bromeon
Re: SFML Move Semantics
« Reply #18 on: July 09, 2020, 10:27:53 pm »
Those suggestions sound good. There are fortunately many classes in SFML, for which the default semantics work well.

But we need to be careful: sf::Image for example requires no special member functions at the moment (although there's a destructor doing nothing). However, a class with no special member functions declared is subject to implictly generated special member functions (aka Big Five). This would break in C++11, as a newly introduced move constructor does a member-wise move of each field, leaving the std::vector and sf::Vector2u fields inconsistent to each other. Ironically, the no-op destructor -- since declared -- prevents exactly that: sf::Image has no move ctor, even if compiled with a C++11 compiler.

Regarding my PR, I'd like to get some feedback, also from other  SFML team members ;)
Zloxx II: action platformer
Thor Library: particle systems, animations, dot products, ...
SFML Game Development:

Nexus

  • Moderator
  • Hero Member
  • *****
  • Posts: 6287
  • Thor Developer
    • View Profile
    • Bromeon
Re: SFML Move Semantics
« Reply #19 on: July 26, 2020, 11:20:36 am »
Regarding my PR, I'd like to get some feedback, also from other  SFML team members ;)

Anyone? :)
(also SFML users, not just team)
Zloxx II: action platformer
Thor Library: particle systems, animations, dot products, ...
SFML Game Development:

Xiaohu

  • Newbie
  • *
  • Posts: 6
    • View Profile
Re: SFML Move Semantics
« Reply #20 on: August 01, 2020, 11:43:49 pm »
I can be biaised by what I need, but I suggest to first add move semantic for simple classes, that is which do not need custom logic.

Which method to use may be important for consistency, but maybe there should be no more than few tens of classes that supports move semantics, so rewriting style from one method to another may take not long.

Most of the work is to identify which classes need custom move logic (I think mostly Handles/Resources), and which ones don't. In this case, it should be possible to make a prototype to follow about the style and conventions, edge-cases classes could be modified after.
« Last Edit: August 01, 2020, 11:48:45 pm by Xiaohu »

Nexus

  • Moderator
  • Hero Member
  • *****
  • Posts: 6287
  • Thor Developer
    • View Profile
    • Bromeon
Re: SFML Move Semantics
« Reply #21 on: August 03, 2020, 11:10:48 pm »
I can be biaised by what I need, but I suggest to first add move semantic for simple classes, that is which do not need custom logic.
For those, the compiler-generated move constructor/operator= would already do the correct thing, or am I misunderstanding you?

Which method to use may be important for consistency, but maybe there should be no more than few tens of classes that supports move semantics, so rewriting style from one method to another may take not long.
Yeah, since there hasn't been much feedback so far, I would suggest we choose one style and go for it. Doing it consistently still makes sense though. It's confusing for everyone if every move ctor/op= is implemented with a different idiom, and it creates needless work to refactor.
Zloxx II: action platformer
Thor Library: particle systems, animations, dot products, ...
SFML Game Development:

unlight

  • Newbie
  • *
  • Posts: 33
    • View Profile
    • Email
Re: SFML Move Semantics
« Reply #22 on: August 13, 2020, 01:55:09 am »
SFML Devs: Can we please get some final input on the topic, or start making some progress on the PRs? Not sure if there is anything else I can do.

Xiaohu

  • Newbie
  • *
  • Posts: 6
    • View Profile
Re: SFML Move Semantics
« Reply #23 on: August 20, 2020, 01:47:30 am »
I guess everybody including SFML Devs has holidays ^^
« Last Edit: August 20, 2020, 01:49:35 am by Xiaohu »

reconn

  • Newbie
  • *
  • Posts: 5
    • View Profile
    • Email
Re: SFML Move Semantics
« Reply #24 on: December 05, 2021, 10:04:14 pm »
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.
« Last Edit: December 05, 2021, 10:07:20 pm by reconn »