Add `sf::UniqueResource` to simplify the implementation of move semantics

[ChrisThrasher]

Description

sf::UniqueResource is a class template that owns a handle to a resource in such a way that you can safely move that handle while ensuring that you never free a given resource more than once. Inspired by std::experimental::unique_resource.

This has potential to be used in a number of place. Whether this is worth merging largely hinges on whether it can be used in 3 or more places.

Places to consider using sf::UniqueResource:

• sf::Texture (Won't let us remove custom copy operations but move operations are simplified)
• sf::Shader
• sf::VertexBuffer
• sf::SoundBuffer (Doesn't let us entirely remove destructor)
• sf::AudioDevice (Doesn't let us entirely remove destructor)
• sf::SoundSource (This potentially breaks the assumption that 0-value handles represent a "null" state but maybe I'm misunderstanding OpenAL)
• XFreePixmap and other X11 resource cleanup functions (This is possible but painfully verbose for little gain other than purity and satisfaction)
• ~~sf::priv::WindowImplWin32~~ (std::unique_ptr is better fit for m_icon since it's a pointer type and the other members need destruction logic too complicated to capture in a single deleter function)
• ~~sf::Font::FontHandles~~ (std::unique_ptr is a better fit since these are pointer types but that's also not possible because other Freetype APIs need to take the address of the underlying pointer which is not possible)

[codecov[bot]]

Codecov Report

Merging #2280 (b5d827a) into master (157feec) will increase coverage by 0.15%. Report is 1 commits behind head on master. The diff coverage is 65.45%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2280      +/-   ##
==========================================
+ Coverage   30.65%   30.80%   +0.15%     
==========================================
  Files         229      230       +1     
  Lines       19748    19734      -14     
  Branches     4723     4703      -20     
==========================================
+ Hits         6053     6079      +26     
+ Misses      13183    13127      -56     
- Partials      512      528      +16     

Files Changed	Coverage Δ
include/SFML/Graphics/Shader.hpp	66.66% <0.00%> (-33.34%)	:arrow_down:
include/SFML/Graphics/VertexBuffer.hpp	0.00% <0.00%> (ø)
src/SFML/Graphics/VertexBuffer.cpp	0.00% <0.00%> (ø)
src/SFML/Graphics/Shader.cpp	27.28% <46.15%> (-0.31%)	:arrow_down:
src/SFML/Graphics/RenderTexture.cpp	31.94% <50.00%> (ø)
src/SFML/Graphics/Texture.cpp	72.50% <76.92%> (+4.06%)	:arrow_up:
include/SFML/Graphics/Texture.hpp	87.50% <85.71%> (-12.50%)	:arrow_down:
include/SFML/System/UniqueResource.inl	100.00% <100.00%> (ø)

---

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 157feec...b5d827a. Read the comment docs.

[kimci86]

Whether this is worth merging largely hinges on whether it can be used in 2 or more places.

Having a quick look at public headers, I think it could be used to manage OpenAL identifiers in SoundBuffer, SoundSource and SoundStream classes and OpenGL identifiers in Shader, Texture, and VertexBuffer classes. It could also be used as a cleaner alternative to std::unique_ptr<std::remove_pointer_t<T>, D> to manage FreeType handles in Font.cpp.

[JonnyPtn]

Personally not sure the benefits of this are worth it, it's still an experimental proposal and adds unnecessary complexity.

Don't feel too strongly though - would rather we just get a decision made quickly so this doesn't continue to hold up other work

[ChrisThrasher]

I rebased on master and a few more CI issues. There's still the bigger problem of why sf::Shader is moveable on all platforms except Windows. I'm not sure why a type would moveable on one platform but not the other.

I'm doing enough to maintain this PR so it doesn't go stale but I still haven't gotten back to putting any serious thought into it.

[ChrisThrasher]

Periodic maintenance rebase. No changes made.

[ChrisThrasher]

https://github.com/SFML/SFML/blob/839ad6cf70276c6ee78a2f7adc5555e8fc07a6bb/src/SFML/Window/Unix/WindowImplX11.cpp#L625

X11 includes a few non-pointer types that need special cleanup functions. This could benefit from sf::UniqueResource as well.

[ChrisThrasher]

I'm thinking about this more and I think I want to focus on making this more similar to std::unique_ptr for a few reasons.

1. std::experimental::unique_resource is not even standardized so it seems excessive to try to match its interface. It was a good starting point but I'm not going to let that stop me from making different decisions.

2. std::unique_ptr has proven itself to be a great interface and one that most C++ programmers understand quite well. It's probably the most common move-only type we work with. If we match its semantics, we're replicating a known, good solution for resource management. While these resource handles aren't exactly equivalent to pointers, they're used in a similar manner so I think it's a reasonable parallel to draw.

So how do we get closer to std::unique_ptr? First we need some robust way to represent a null state for the resource handle. One option is to treat the default-constructed state of T as representing null. This is what std::unique_ptr does because we've all collectively agreed that pointer types use their default-constructed (or "zero") state to mean "there is no value". In other words std::unique_ptr depends on a sentinel value to represent a default initialized or moved-from state.

sf::UniqueResource aims to work with arbitrary resource handle types. Those resource handle types often do use the default-constructed state (aka the value 0) to represent a null state but this is not a safe assumption to make in the general case. Because of this we maybe cannot use a sentinel value to represent an empty or moved-from state. Luckily we can use std::optional within the class to bolt on an "empty" state to any arbitrary T.

At this point we can now mostly copy the std::unique_ptr interface. We can use operator* and operator-> to easily access (but not modify) the contained handle and modify it as needed. We can have .get() which returns an std::optional<T> just like std::unique_ptr::get() returns the raw pointer it's storing inside. We have operator bool to easily query whether the unique resource has a value. We match the semantics of std::unique_ptr wherein a non-null pointer/resource indicates that the pointer owns something. This eliminates issues where someone tries to use the underlying resource handle after it has been moved. .release() and .reset() still work as intended when you need to free the current resource or replace the current handle with a handle to a new resource.

There's a lot of value in a type that already obeys existing semantics we all know and understand.

EDIT: I chose to not add operator* and operator-> because "defreferencing a unique resource" is not an operation that exists. There is nothing in the type system that represents the handled resource, hence the existence of the handle. Syntactic sugar to access to underlying handle is not useful when .get() already exists. While sf::UniqueResource will have some ergonomic similarities to std::unique_ptr it's not reasonable to draw such a close parallel between the two.

[ChrisThrasher]

I've been working on this off and on the last few weeks and pushed up an updated version of sf::UniqueResource alongside using it in sf::Shader and sf::Texture. I'm still on the fence about whether the underlying handle should be nullable via std::optional or nullable by using its value-initialized state as the "null" state (like std::unique_ptr does) but so far I'm sticking with the std::optional<Handle> route. If we use sf::UniqueResource in a bunch of places and it still ends up that all handles use 0 as their "null" state then we can remove the std::optional and simplify sf::UniqueResource in a few ways. However if a single kind of resource handle uses 0 as a valid state then we can't do that.

[ChrisThrasher]

I was able to use UniqueResource to remove the custom destructors for sf::Shader, sf::Texture, and sf::VertexBuffer which means we've met my minimum requirement for this being a worthwhile addition to the library! After doing that I saw that every single resource we were managing used 0 as its null state so I added one more commit to this PR that takes advantage of that fact and removes std::optional from UniqueResource in favor of treating the Handle's value-initialized state as being its "null" state. This lets us simplify things significantly.

Are there resources with handles where the value-initialized state is valid? Yeah maybe. However those handles are either not used in SFML or if they are used it's not a big deal to maintain the status quo and handle them in a more manual fashion. This most recent change means that the majority of resource handles are treated in a simple, elegant fashion that is very similar to std::unique_ptr.

I know I've changed my mind a few times on this PR but this is the most certain about it I've been so far. I'm marking this as ready for review so we can resume the discussion in hopes of getting this merged soon.

[Bromeon]

So how do we get closer to std::unique_ptr? First we need some robust way to represent a null state for the resource handle.

Do we though? I found unique_ptr being an owned pointer and representing null state a bad separation of responsibilities. It's basically the Billion Dollar Mistake.

I wonder if sf::UniqueResource should not have an always-valid state (barring moved-from), and use std::optional<sf::UniqueResource<T>> if we need an absent value.

Maybe the use cases we handle become too verbose as a result, though; in that case, might be indeed better to have built-in nullability.

[ChrisThrasher]

Do we though?

I reverted that decision between writing that comment and the latest commit. My new preferred solution involves no optionals, at least not internal to UniqueResource.

Maybe the use cases we handle become too verbose as a result, though; in that case, might be indeed better to have built-in nullability.

The built-in nullability comes from the handles themselves being nullable as per the conventional of the handles we're wrapping. The libraries giving us these handles treat 0 as "null" so we can take advantage of that.

[ChrisThrasher]

Rebased on master and fixed conflicts

[kimci86]

If we require that a default-constructed handle corresponds to a null state, maybe UniqueResource needs to be in sf::priv namespace as it cannot be used in the general case? Anyways this convention could be added to the class documentation.

[ChrisThrasher]

If we require that a default-constructed handle corresponds to a null state, maybe UniqueResource needs to be in sf::priv namespace as it cannot be used in the general case?

I'm open to this if others want to do it. I don't expect users to use this type since it's a pretty niche tool that only matters to us working with 3rd party C APIs. My intention is to clean up some of our implementations and not to provide any additional convenience to users. We can always change our mind and make it public later if we so choose. One downside is that it becomes a bit weird for us to have tests for it since so far we have zero tests for anything within sf::priv. I'm not keen on dropping those tests which makes me hesitant to put it in sf::priv.

Anyways this convention could be added to the class documentation.

Done.

[Bromeon]

If we require that a default-constructed handle corresponds to a null state, maybe UniqueResource needs to be in sf::priv namespace as it cannot be used in the general case?

If this is at some point no longer the case, we could always have compile-time metaprogramming that would construct a null state for a handle type.

Either something like

template <typename T>
struct HandleDefault {
    static T value() { return T(); }
};

// now specialize for concrete T

Or if we have lots of handles that are int but fundamentally different types:

template <typename T, typename HandleOp>
class UniqueResource<T, HandleOp = Default<T>> { ... };

---

If the type is only used internally, it should probably be in sf::priv. Then we can also iterate on the API, including breaking changes, until we converge on something that works well for a lot of cases. It could also be promoted to the public API as soon as we either want to expose UniqueResource in interfaces, or want the user to benefit from similar functionality.