6
\$\begingroup\$

I wrote a little library to generate wrappers for SDL2 in C++20 with the idea to build a simple video game with it afterwards, and was looking for some feedback on the code!

The idea is not to create new classes (this will be done at a later stage and higher level), but just to wrap the SDL2 functions into functions that:

  • check errors,
  • return unique pointers to automatically free the resources,
  • be called exactly as in the documentation (I don't want to have to do unique_ptr.get() when I call a function on my window),
  • work with functions that return void, a pointer, an error code (0: success, <0: error) or an integer.

Basically, taking https://eb2.co/blog/2014/04/c-14-and-sdl2-managing-resources/ one step further and nearly automatically creating the wrappers.

Here is the code of the library (I have not wrapped all the functions of SDL yet, only some to make a window appear):

#pragma once

#include <SDL.h>
#include <stdexcept>
#include <memory>

namespace {
    // Creates a deleter for unique_ptr from a function
    template<auto FUN>
    class Deleter {
    public:
        template<typename T>
        constexpr void operator()(T *arg) const {
            FUN(arg);
        }
    };

    // Creates a deleter for unique_ptr which does nothing
    class NoDeleter {
    public:
        template<typename T>
        constexpr void operator()(T *arg) const {}
    };

    // Helper class to get a T* from a unique_ptr<T>, and itself from everything else
    template<typename T>
    class AsRaw {
    public:
        static constexpr auto call(auto &&t) {
            return t;
        }
    };

    template<typename T, typename D>
    class AsRaw<std::unique_ptr<T, D> &> {
    public:
        static constexpr auto call(std::unique_ptr<T, D> &t) {
            return t.get();
        }
    };

    template<typename T, typename D>
    class AsRaw<std::unique_ptr<T, D> &&> {
    public:
        static constexpr auto call(std::unique_ptr<T, D> &&t) {
            return t.get();
        }
    };

    template<typename T, typename D>
    class AsRaw<const std::unique_ptr<T, D> &> {
    public:
        static constexpr auto call(const std::unique_ptr<T, D> &t) {
            return t.get();
        }
    };

    template<typename T>
    auto as_raw(T &&t) {
        return AsRaw<T>::call(std::forward<T>(t));
    }

    // Forward-declaration only, used for decltype to get the return type of a function pointer
    template<typename OUT, typename ...INS>
    OUT return_type(OUT (*)(INS...));
}

namespace sdl {
    /**
     * The wrapper
     * @tparam FUN The function to wrap
     * @tparam CHECK Whether to check the return value for errors (only used if the return type is not void)
     */
    template<auto FUN, bool CHECK = true>
    class Wrapper {
        using out_t = decltype(return_type(FUN));
    public:
        constexpr void operator()(auto &&... arguments) const requires(std::is_void_v<out_t>) {
            FUN(std::forward<decltype(as_raw(arguments))>(as_raw(arguments))...);
        }

        constexpr out_t operator()(auto &&... arguments) const requires (!CHECK &&
                                                                         !std::is_void_v<out_t>) {
            return FUN(std::forward<decltype(as_raw(arguments))>(as_raw(arguments))...);
        }

        constexpr out_t operator()(auto &&... arguments) const requires (CHECK &&
                                                                         std::is_pointer_v<out_t>) {
            auto res = FUN(std::forward<decltype(as_raw(arguments))>(as_raw(arguments))...);

            if (res == nullptr) {
                throw std::runtime_error(SDL_GetError());
            }

            return res;
        }

        constexpr out_t operator()(auto &&... arguments) const requires (CHECK &&
                                                                         std::is_integral_v<out_t> &&
                                                                         std::is_signed_v<out_t>) {
            auto res = FUN(std::forward<decltype(as_raw(arguments))>(as_raw(arguments))...);

            if (res < 0) {
                throw std::runtime_error(SDL_GetError());
            }

            return res;
        }
    };

    /**
     * Secondary wrapper to change the return type of the function
     * @tparam WRAPPER Wrapped function
     * @tparam OUT The new return type
     */
    template<typename WRAPPER, typename OUT>
    class TypeWrapper {
    public:
        constexpr OUT operator()(auto &&... arguments) const {
            return OUT{WRAPPER{}(std::forward<decltype(arguments)>(arguments)...)};
        }
    };

    // The SDL Wrappers
    constexpr Wrapper<SDL_Init> init;
    using Window = std::unique_ptr<SDL_Window, Deleter<SDL_DestroyWindow>>;
    constexpr TypeWrapper<Wrapper<SDL_CreateWindow, true>, Window> create_window;
    using WindowSurface = std::unique_ptr<SDL_Surface, NoDeleter>;
    constexpr TypeWrapper<Wrapper<SDL_GetWindowSurface>, WindowSurface> get_window_surface;
    constexpr Wrapper<SDL_FillRect> fill_rect;
    constexpr Wrapper<SDL_MapRGB, false> map_rgb;
    constexpr Wrapper<SDL_UpdateWindowSurface> update_window_surface;
    constexpr Wrapper<SDL_PollEvent, false> poll_event;
    constexpr Wrapper<SDL_Quit> quit;

    // Just a simple Context class to use RAII for init and quit
    class Context {
    public:
        Context() {
            sdl::init(SDL_INIT_VIDEO);
        }

        ~Context() {
            sdl::quit();
        }
    };
}

And the code to create a window:

#include "sdl.hh"

constexpr int SCREEN_WIDTH = 640;
constexpr int SCREEN_HEIGHT = 480;

int main() {
    sdl::Context context;

    auto window =
            sdl::create_window("SDL Tutorial", SDL_WINDOWPOS_UNDEFINED, SDL_WINDOWPOS_UNDEFINED,
                               SCREEN_WIDTH, SCREEN_HEIGHT, SDL_WINDOW_SHOWN);

    auto screenSurface = sdl::get_window_surface(window);

    sdl::fill_rect(screenSurface, nullptr, sdl::map_rgb(screenSurface->format, 0xFF, 0xFF, 0xFF));

    sdl::update_window_surface(window);

    SDL_Event e;
    bool quit = false;
    while (!quit) {
        while (sdl::poll_event(&e)) {
            if (e.type == SDL_QUIT) quit = true;
        }
    }

    return 0;
}
\$\endgroup\$

2 Answers 2

5
\$\begingroup\$

Things that you can do differently

I don't think there is anything wrong with how you implemented the deleters, but there are two alternatives you could consider. First, you can create custom specializations of std::default_delete, so you don't need to pass a second template paramter to std::unique_ptr:

namespace std {
template<>
struct default_delete<SDL_Window> {
    void operator()(SDL_Window *window) const {
        SDL_DestroyWindow(window);
    }
};
}

namespace sdl {
    using Window = std::unique_ptr<SDL_Window>;
    …
};

This doesn't save a lot of code though. Another option is to write custom deleter classes entirely, by using the power of lambdas. Consider:

template<auto FUN>
using Deleter = decltype([](auto* arg){FUN(arg);});
using NoDeleter = decltype([](auto*){});

Or you could even pass a lambda type directly to std::unique_ptr:

using Window = std::unique_ptr<SDL_Window, decltype([](auto* w){SDL_DestroyWindow(w);})>;

Although it's probably nicer to keep the Deleter and NoDeleter types, it makes the declarations of the SDL object types cleaner.

Avoid code duplication

There is some code duplication in Wrapper that you might be able to reduce. For example, instead of using template overloads, you can use if constexpr to check if a return value needs to be checked, and if so how.

Do you need all the overloads of AsRaw?

Since SDL is a C library, it has no concept of rvalues, so it seems a bit pointless to have an overload for that in AsRaw.

AsRaw potentially unwraps too much

AsRaw will unwrap all std::unique_ptrs, even those not created by your type aliases. Considering that some SDL functions take void* parameters, it could happen that someone passes their own std::unique_ptr, and now it will incorrectly get unwrapped. While it most likely would be a bug, you've now caused the compiler to not catch that bug anymore.

To do this, you might want to make Window and WindowSurface distinct types rather than type aliases for std::unique_ptr, and then AsRaw can specialize on those.

\$\endgroup\$
4
  • \$\begingroup\$ Thank you so much for the review! I wasn't aware of the lambda in decltype syntax, that is so clean. Regarding the rvalue overload, you made me think about it and it probably is even harmful to have it, as the std::unique_ptr will be destroyed when leaving the function but the pointer is returned and becomes dangling (which is not a problem with the lvalue references as they are not destroyed). And for AsRaw, do you think it would make sense to create a class like SDLPointer that extends unique_ptr (and basically replace all occurrences of unique_ptr by SDLPointer) ? \$\endgroup\$
    – Alex
    Feb 25, 2023 at 16:38
  • 1
    \$\begingroup\$ Also, for the code duplication, it looks like we are allowed to return a "void value" in a void function (what I mean is that void foo(){} void bar() {return foo(); } works), so this would allow to merge the code when there is no check and when the return type is void, but would this be a bad practice? Cppreference says In a function returning (possibly cv-qualified) void, the return statement with expression can be used, if the expression type is (possibly cv-qualified) void., but I have never seen this used \$\endgroup\$
    – Alex
    Feb 25, 2023 at 16:57
  • 1
    \$\begingroup\$ If it leads to less code duplication, I would certainly use the fact that you can return void. \$\endgroup\$
    – G. Sliepen
    Feb 25, 2023 at 19:06
  • \$\begingroup\$ I'll definitely use that then. Also, please disregard my previous statement about the dangling pointer, I lost myself about when the value would be deleted, in this case it works, but we should always remain careful with that kind of operations \$\endgroup\$
    – Alex
    Feb 25, 2023 at 19:34
4
\$\begingroup\$

Design review

Wrapping SDL is not only a good idea, it is a great practice project for learning SDL and C++. Your take on it is unique and interesting, and might make for an interesting challenge, but I’m not sure it’s the smart way to wrap a library.

When wrapping any library—particularly C libraries—you need to consider the question of migration:

  1. You don’t care about how much it looks like the unwrapped library (but it should be vaguely similar; you’re not writing a whole new API).
  2. It should look as much like the unwrapped library as possible (but small differences are okay), except be safer and better.

This is a very important difference to consider, because it fundamentally impacts how you write your library. Let me illustrate.

Suppose you’re starting with a pretty bog-standard SDL sample. (I’m using the LazyFoo example, modified to be C++ rather than C.)

#include <cstdlib>
#include <iostream>
#include <stdexcept>
#include <string>

#include <SDL2/SDL.h>

auto main() -> int
{
    using namespace std::literals;

    constexpr auto screen_width  = 640;
    constexpr auto screen_height = 480;

    try
    {
        if (SDL_Init(SDL_INIT_VIDEO) < 0)
            throw std::runtime_error{"SDL could not initialize! SDL_Error: "s + SDL_GetError()};

        try
        {
            auto const window = SDL_CreateWindow("SDL Tutorial", SDL_WINDOWPOS_UNDEFINED, SDL_WINDOWPOS_UNDEFINED, screen_width, screen_height, SDL_WINDOW_SHOWN);
            if (not window)
                throw std::runtime_error{"Window could not be created! SDL_Error: "s + SDL_GetError()};

            try
            {
                auto const screenSurface = SDL_GetWindowSurface(window);

                SDL_FillRect(screenSurface, nullptr, SDL_MapRGB(screenSurface->format, 0xFFu, 0xFFu, 0xFFu));

                SDL_UpdateWindowSurface(window);

                //Hack to get window to stay up
                SDL_Event e; bool quit = false; while( quit == false ){ while( SDL_PollEvent( &e ) ){ if( e.type == SDL_QUIT ) quit = true; } }

                SDL_DestroyWindow(window);
            }
            catch (...)
            {
                SDL_DestroyWindow(window);
                throw;
            }

            SDL_Quit();
        }
        catch (...)
        {
            SDL_Quit();
            throw;
        }
    }
    catch (std::exception const& x)
    {
        std::cout << "[ERROR]: " << x.what() << '\n';
        return EXIT_FAILURE;
    }
    catch (...)
    {
        std::cout << "[ERROR]: " << "unknown error" << '\n';
        return EXIT_FAILURE;
    }
}

The question is:

  • Do you want to throw all this out and rewrite it from scratch, probably making it completely different in the process? OR…
  • Do you want to gradually transition to safer code?

If you just want to throw out the old code and start over… fine, then you can do pretty much whatever you like. But it sure looks like you want to make gradual migration possible. In that case, you need to think very carefully about how to do that transition… and it will have a major impact on the look of your final wrapper code.

A good first might be to write simple wrappers for all SDL functions, in a new namespace. So, every single function SDL_WhatEver(...) would become sdl::wrap::WhatEver(...), which would just be defined as:

namespace sdl::wrap {

inline auto WhatEver(...)
{
    return ::SDL_WhatEver(...);
}

} // namespace sdl::wrap

If you do that for all SDL functions, macros, and types, you can just do a find/replace on an entire SDL project, and replace SDL_ with sdl::wrap::, and the project should still compile and work as it did before.

This seems to be what you’re aiming for… but you want to automate this. Okay, so, maybe what you want is something more like:

#include <concepts>
#include <cstdlib>
#include <iostream>
#include <stdexcept>
#include <string>
#include <utility>

#include <SDL2/SDL.h>

namespace sdl {

// Unfortunately, we have to pay the cost of the hidden this pointer every
// time we call a wrapped function. This will go away in C++23, where we can
// make operator() static, and instead of using inline constexpr objects as
// the wrapped things, we can just use aliases.
template <auto Func>
struct wrapper
{
    template <typename... Args>
        requires std::invocable<decltype(Func), Args...>
    constexpr auto operator()(Args&&... args) const -> decltype(auto)
    {
        return Func(std::forward<Args>(args)...);
    }
};

#define INDI_SDL_wrap_func(func) inline constexpr auto func = wrapper<::SDL_ ## func>{};

INDI_SDL_wrap_func(Init)
INDI_SDL_wrap_func(Quit)

INDI_SDL_wrap_func(GetError)

INDI_SDL_wrap_func(CreateWindow)
INDI_SDL_wrap_func(DestroyWindow)

INDI_SDL_wrap_func(GetWindowSurface)
INDI_SDL_wrap_func(UpdateWindowSurface)

INDI_SDL_wrap_func(FillRect)
INDI_SDL_wrap_func(MapRGB)

} // namespace sdl

auto main() -> int
{
    using namespace std::literals;

    constexpr auto screen_width  = 640;
    constexpr auto screen_height = 480;

    try
    {
        if (sdl::Init(SDL_INIT_VIDEO) < 0)
            throw std::runtime_error{"SDL could not initialize! SDL_Error: "s + sdl::GetError()};

        try
        {
            auto const window = sdl::CreateWindow("SDL Tutorial", SDL_WINDOWPOS_UNDEFINED, SDL_WINDOWPOS_UNDEFINED, screen_width, screen_height, SDL_WINDOW_SHOWN);
            if (not window)
                throw std::runtime_error{"Window could not be created! SDL_Error: "s + sdl::GetError()};

            try
            {
                auto const screenSurface = sdl::GetWindowSurface(window);

                sdl::FillRect(screenSurface, nullptr, sdl::MapRGB(screenSurface->format, 0xFFu, 0xFFu, 0xFFu));

                sdl::UpdateWindowSurface(window);

                //Hack to get window to stay up
                SDL_Event e; bool quit = false; while( quit == false ){ while( SDL_PollEvent( &e ) ){ if( e.type == SDL_QUIT ) quit = true; } }

                sdl::DestroyWindow(window);
            }
            catch (...)
            {
                sdl::DestroyWindow(window);
                throw;
            }

            sdl::Quit();
        }
        catch (...)
        {
            sdl::Quit();
            throw;
        }
    }
    catch (std::exception const& x)
    {
        std::cout << "[ERROR]: " << x.what() << '\n';
        return EXIT_FAILURE;
    }
    catch (...)
    {
        std::cout << "[ERROR]: " << "unknown error" << '\n';
        return EXIT_FAILURE;
    }
}

Now, the next step is to make your wrappers a bit smarter. Let’s start with a simple unwrap function:

template <typename T>
constexpr auto unwrap(std::remove_reference_t<T>& t) noexcept -> T&&
{
    return std::forward<T>(t);
}

template <typename T>
constexpr auto unwrap(std::remove_reference_t<T>&& t) noexcept -> T&&
{
    return std::forward<T>(t);
}

template <auto Func>
struct wrapper
{
    template <typename... Args>
        requires std::invocable<decltype(Func), Args...>
    constexpr auto operator()(Args&&... args) const -> decltype(auto)
    {
        return Func(unwrap<Args>(args)...);
    }
};

Next we need a concept to check for whether unwrapping is necessary:

template <typename T>
struct is_smart_pointer : std::false_type {};

template <typename T, typename Deleter>
struct is_smart_pointer<std::unique_ptr<T, Deleter>> : std::true_type {};

// Also for shared_ptr, etc.

template <typename T>
inline constexpr auto is_smart_pointer_v = is_smart_pointer<T>::value;

template <typename T>
concept wrapped_type =
    is_smart_pointer_v<T>
    // ... add other possibilities later, if desired...
;

And now we can add some unwrapping:

template <typename T>
constexpr auto unwrap(std::remove_reference_t<T>& t) noexcept -> decltype(auto)
    // Note return type has changed!
{
    using type = std::remove_cvref_t<T>;

    if constexpr (wrapped_type<type>)
    {
        if constexpr (is_smart_pointer_v<type>)
        {
            return std::forward<T>(t).get();
        }
        else
        {
            static_assert(always_false<T>, "unknown wrapped type");
        }
    }
    else
    {
        return std::forward<T>(t);
    }
}

// Same for &&
//
// Unfortunately, C++ requires we duplicate the function for lvalue and rvalue
// references. But to avoid duplication, you could make the body a macro.

That should be all we need.

One more thing we’ll need, to make sure the constraints still work, is a type trait to extract the unwrapped type. (If you don’t care about constraining the wrapper, then you won’t need this.)

template <typename T>
struct unwrapped_type { using type = T; };
template <typename T>
    requires wrapped_type<std::remove_cvref_t<T>>
struct unwrapped_type<T> { using type = decltype(unwrap<T>(std::declval<T>())); };

template <typename T>
using unwrapped_type_t = unwrapped_type<T>::type;

Which gives us:

#include <concepts>
#include <cstdlib>
#include <iostream>
#include <memory>
#include <stdexcept>
#include <string>
#include <type_traits>
#include <utility>

#include <SDL2/SDL.h>

namespace sdl {

template <typename T>
inline constexpr auto always_true = true;
template <typename T>
inline constexpr auto always_false = false;

template <typename T>
struct is_smart_pointer : std::false_type {};

template <typename T, typename Deleter>
struct is_smart_pointer<std::unique_ptr<T, Deleter>> : std::true_type {};

// Also for shared_ptr, etc.

template <typename T>
inline constexpr auto is_smart_pointer_v = is_smart_pointer<T>::value;

template <typename T>
concept wrapped_type =
    is_smart_pointer_v<T>
    // ... add other possibilities later, if desired...
;

template <typename T>
constexpr auto unwrap(std::remove_reference_t<T>& t) noexcept -> decltype(auto)
{
    using type = std::remove_cvref_t<T>;

    if constexpr (wrapped_type<type>)
    {
        if constexpr (is_smart_pointer_v<type>)
        {
            return std::forward<T>(t).get();
        }
        else
        {
            static_assert(always_false<T>, "unknown wrapped type");
        }
    }
    else
    {
        return std::forward<T>(t);
    }
}

template <typename T>
constexpr auto unwrap(std::remove_reference_t<T>&& t) noexcept -> decltype(auto)
{
    using type = std::remove_cvref_t<T>;

    if constexpr (wrapped_type<type>)
    {
        if constexpr (is_smart_pointer_v<type>)
        {
            return std::forward<T>(t).get();
        }
        else
        {
            static_assert(always_false<T>, "unknown wrapped type");
        }
    }
    else
    {
        return std::forward<T>(t);
    }
}

template <typename T>
struct unwrapped_type { using type = T; };
template <typename T>
    requires wrapped_type<std::remove_cvref_t<T>>
struct unwrapped_type<T> { using type = decltype(unwrap<T>(std::declval<T>())); };

template <typename T>
using unwrapped_type_t = unwrapped_type<T>::type;

template <auto Func>
struct wrapper
{
    template <typename... Args>
        requires std::invocable<decltype(Func), unwrapped_type_t<Args>...>
    constexpr auto operator()(Args&&... args) const -> decltype(auto)
    {
        return Func(unwrap<Args>(args)...);
    }
};

#define INDI_SDL_wrap_func(func) inline constexpr auto func = wrapper<::SDL_ ## func>{};

INDI_SDL_wrap_func(Init)
INDI_SDL_wrap_func(Quit)

INDI_SDL_wrap_func(GetError)

INDI_SDL_wrap_func(CreateWindow)
INDI_SDL_wrap_func(DestroyWindow)

INDI_SDL_wrap_func(GetWindowSurface)
INDI_SDL_wrap_func(UpdateWindowSurface)

INDI_SDL_wrap_func(FillRect)
INDI_SDL_wrap_func(MapRGB)

} // namespace sdl

// Need this temporarily to avoid a double-delete.
struct null_deleter
{
    constexpr auto operator()(auto*) const noexcept {}
};

auto main() -> int
{
    using namespace std::literals;

    constexpr auto screen_width  = 640;
    constexpr auto screen_height = 480;

    try
    {
        if (sdl::Init(SDL_INIT_VIDEO) < 0)
            throw std::runtime_error{"SDL could not initialize! SDL_Error: "s + sdl::GetError()};

        try
        {
            auto const window = sdl::CreateWindow("SDL Tutorial", SDL_WINDOWPOS_UNDEFINED, SDL_WINDOWPOS_UNDEFINED, screen_width, screen_height, SDL_WINDOW_SHOWN);
            if (not window)
                throw std::runtime_error{"Window could not be created! SDL_Error: "s + sdl::GetError()};

            try
            {
                // NOTE: I added this (unnecessary) variable just to prove
                // that migration from raw pointers to smart pointers is
                // transparent.
                //
                // You can see I call GetWindowSurface() and
                // UpdateWindowSurface() with the smart pointer, and
                // DestroyWindow() with the raw pointer.
                auto const p_window = std::unique_ptr<::SDL_Window, null_deleter>{window};

                auto const screenSurface = sdl::GetWindowSurface(p_window);

                sdl::FillRect(screenSurface, nullptr, sdl::MapRGB(screenSurface->format, 0xFFu, 0xFFu, 0xFFu));

                sdl::UpdateWindowSurface(p_window);

                //Hack to get window to stay up
                SDL_Event e; bool quit = false; while( quit == false ){ while( SDL_PollEvent( &e ) ){ if( e.type == SDL_QUIT ) quit = true; } }

                sdl::DestroyWindow(window);
            }
            catch (...)
            {
                sdl::DestroyWindow(window);
                throw;
            }

            sdl::Quit();
        }
        catch (...)
        {
            sdl::Quit();
            throw;
        }
    }
    catch (std::exception const& x)
    {
        std::cout << "[ERROR]: " << x.what() << '\n';
        return EXIT_FAILURE;
    }
    catch (...)
    {
        std::cout << "[ERROR]: " << "unknown error" << '\n';
        return EXIT_FAILURE;
    }
}

This is about as far as you can go with a simple, automatic wrapper regime, because from this point on, you will either require structural changes to the actual program, or there is some dangerous complexity that needs to be taken into consideration.

The SDL_Init()/SDL_Quit() problem

Some functions can’t be simply wrapped and done with, and the most obvious culprit is the SDL_Init()/SDL_Quit() pair.

What you want is to transform:

    try
    {
        if (sdl::Init(SDL_INIT_VIDEO) < 0)
            throw std::runtime_error{...};

        try
        {
        // ... rest of program ...

            sdl::Quit();
        }
        catch (...)
        {
            sdl::Quit();
            throw;
        }
    }
    catch (std::exception const& x)
    {
        // ... error handling ...
    }

… into:

    try
    {
        auto context = sdl::Init(SDL_INIT_VIDEO);
        // Implicit throw on error.

        // ... rest of program ...

        // No need for SDL_Quit().
    }
    catch (std::exception const& x)
    {
        // ... error handling ...
    }

But there is no way to get from here to there without rewriting the program code. Even if you have sdl::Init(...) return a smart, RAII object, the way it is used (inside an if check) means it can’t work as a way to RAII the whole SDL system (because the object would be destructed at the end of the if check, not the end of the entire scope).

The problem with these functions is that, because they don’t use a token to keep track of themselves (that is, something returned from SDL_Init(), and consumed by SDL_Quit()), they don’t need to be structurally paired in any way. They can appear anywhere in code, so long as only one of each gets called in a single run.

So there’s just no way to transparently wrap SDL_Init()/SDL_Quit(). Migration is going to require some pain. The question is how much.

Well, one option is to simply wrap SDL_Init()/SDL_Quit(), but mark them as [[deprecated]]. This will be annoying, but it will alert the programmer to the problem.

If you don’t want to mark them deprecated (so that migration is less painful), another option is to wrap them, but have the wrapped init function increment a counter, and the wrapped quit function decrement a counter, and print errors if initialization happens with a non-zero counter, or if quitting does not return the counter to zero. Then create your RAII object, and have that also increment/decrement the counter, and error if the count is wrong. This will mean that when you replace the raw functions with the RAII object, if there is any imbalance, or any init/quit calls hidden anywhere strange, they will trigger errors. It’s not great, because it means run-time errors, rather than compile-time errors. But it’s probably the best you can do.

Wrapping destroy functions

You have to be super, super careful wrapping functions that release resources, like SDL_DestroyWindow(). What you want, basically, is an overload pair like:

namespace sdl {

auto DestroyWindow(SDL_Window*) -> void;

template <typename Deleter>
auto DestroyWindow(std::unique_ptr<SDL_Window, Deleter> const&) -> void;

} // namespace sdl

What we have already takes care of this, because the unique_ptr is automatically unwrapped to the raw pointer.

The problem is that we want different behaviour for the two overloads. We want the raw pointer overload to call SDL_DestroyWindow(). But we DON’T want the smart pointer overload to do that, because if it does, then it will double-delete when the smart pointer is ultimately destroyed.

So we cannot do the simple automatic wrap trick with functions that release resources. You maybe need to manually write wrap functions, something like this:

namespace sdl {

auto DestroyWindow(SDL_Window* p) -> void
{
    ::SDL_DestroyWindow(p);
}

template <typename Deleter>
auto DestroyWindow(std::unique_ptr<SDL_Window, Deleter> const&) -> void
{
    // Do nothing.
}

} // namespace sdl

The catch is that now resources won’t be destroyed where they explicitly appear to be… they will be destroyed implicitly later.

If you want explicit releasing to happen when requested, you’d have to do something like:

namespace sdl {

auto DestroyWindow(SDL_Window* p) -> void
{
    ::SDL_DestroyWindow(p);
}

template <typename Deleter>
auto DestroyWindow(std::unique_ptr<SDL_Window, Deleter> const&) -> void
{
    // Do nothing.
}

template <typename Deleter>
auto DestroyWindow(std::unique_ptr<SDL_Window, Deleter>& p) -> void
{
    p.reset();
}

template <typename Deleter>
auto DestroyWindow(std::unique_ptr<SDL_Window, Deleter>&& p) -> void
{
    p.reset();
}

} // namespace sdl

Don’t know how well that will work, though.

Automatic wrapping and checking return values

So, we could be “clever”, and have our automatic wrapper automatically wrap the return value as well. Something like:

struct deleter
{
    auto operator()(::SDL_Window* window) const noexcept
    {
        ::SDL_DestroyWindow(window);
    }

    auto operator()(::SDL_Surface* surface) const noexcept
    {
        ::SDL_FreeSurface(surface);
    }

    // ... and so on ...
};

template <typename T>
using unique_ptr = std::unique_ptr<T, deleter>;

template <auto Func>
struct wrapper
{
    template <typename... Args>
        requires std::invocable<decltype(Func), unwrapped_type_t<Args>...>
    constexpr auto operator()(Args&&... args) const -> decltype(auto)
    {
        using result_type = std::invoke_result_t<decltype(Func), unwrapped_type_t<Args>...>;

        if constexpr (std::is_pointer_v<result_type>)
        {
            return unique_ptr<std::remove_pointer_t<result_type>>{
                Func(unwrap<Args>(args)...)
            };
        }
        else
        {
            return Func(unwrap<Args>(args)...);
        }
    }
};

This is risky, though, because some functions return pointers that are not supposed to be released. In fact, in the simple program I’ve been using, SDL_GetWindowSurface() returns a SDL_Surface*… but that pointer is owned by the window, and should not be released with SDL_FreeSurface(). However, if you create a surface manually with SDL_CreateRGBSurface() or some other function, then it (sometimes) should be released.

So you would need to return a smart pointer from functions like SDL_CreateRGBSurface()… but either a raw pointer or a smart pointer with a null deleter from functions like SDL_GetWindowSurface().

To make things even more complicated, sometimes whether or not the resources should be released may not not a property of the function, but of some other factor. In that case, you’ll need a deleter with a flag of some kind, or a check function, or… who knows.

Now, maybe you can tweak the automatic wrapper generator. Maybe you could do something like:

struct default_wrapper
{
    template <typename T>
    constexpr auto operator()(T&& t) const
    {
        if constexpr (std::is_pointer_v<std::remove_cvref_t<T>>)
            return unique_ptr<std::remove_reference_t<T>>{t};
        else
            return std::forward<T>(t);
    }
};

struct null_delete_wrapper
{
    template <typename T>
    constexpr auto operator()(T* t) const noexcept
    {
        return std::unique_ptr<T, null_deleter>{t};
    }
};

template <auto Func, typename Wrapper = default_wrapper>
struct wrapper
{
    template <typename... Args>
        requires std::invocable<decltype(Func), unwrapped_type_t<Args>...>
    constexpr auto operator()(Args&&... args) const -> decltype(auto)
    {
        return Wrapper{}(Func(unwrap<Args>(args)...));
    }
};

#define INDI_SDL_wrap_func(func) inline constexpr auto func = wrapper<::SDL_ ## func>{};
#define INDI_SDL_wrap_func_with_wrapper(func, wrapper) inline constexpr auto func = wrapper<::SDL_ ## func, wrapper>{};

INDI_SDL_wrap_func(SDL_CreateRGBSurface)
INDI_SDL_wrap_func_with_wrapper(GetWindowSurface, null_delete_wrapper)

But at that point, you’re considering the disposition of the return type on a case-by-case basis anyway, so you might as well be explicit about whether to wrap and what to wrap it with. In other words, you gain nothing from automatic return type wrapping, and instead create the possibility of missing a case (or, when a new function is added to SDL) and creating bugs.

Don’t try to be so clever you screw yourself over. Never thoughtlessly change the semantics of return types. That way lies madness.

Design summary

So how do you move from the unsafe raw pointer API of SDL to a safer C++ API? Carefully.

It’s okay to automatically wrap, say, SDL_UpdateWindowSurface(), and have it intelligently and automatically handle either a SDL_Window* or std::unique_ptr<SDL_Window, sdl::deleter> const&. That’s just overloading. You’re not changing the semantics of the function, just allowing more options for the form of the arguments you call it with. It doesn’t matter what else happens anywhere else in the program, everything will Just Work.

It’s less okay to automatically wrap SDL_CreateWindow(), and change its return type from SDL_Window* to std::unique_ptr<SDL_Window, sdl::deleter>, because now you have changed the semantics of the return type. That changes what the correct operations on the return value are: before, you could call SDL_DestroyWindow() on it… now you can’t. And simply conceptually adding an overload for SDL_DestroyWindow() that takes a smart pointer makes things even more dangerous, because you may end up with double-deletes (or no delete if, for some reason, you temporarily switch back to the unwrapped SDL_CreateWindow()). In other words, you can’t change the semantics of a return type, and you also can’t change the semantics of a function (like making destroy_window() sometimes not destroy a window, such as when passed a smart pointer).

So, in summary:

  1. Having automatic transformations (wrapping/unwrapping) on arguments… mostly fine.

    • EXCEPT for release functions, which should handle raw handles and smart (auto-releasing) handles differently.
  2. Having automatic transformations on return types… very dangerous. Unless you can be sure you’re not changing the semantics, don’t do it.

    If you want a different return type, make a different function (with a different name). That makes it a “noisy” change that forces coders to actually check that it does what they expect.

Code review

#pragma once

Don’t use #pragma once.

#include <SDL.h>
#include <stdexcept>
#include <memory>

I am always wary of including anything before standard header files, but YMMV.

    // Creates a deleter for unique_ptr from a function
    template<auto FUN>
    class Deleter {
    public:
        template<typename T>
        constexpr void operator()(T *arg) const {
            FUN(arg);
        }
    };

Before I look at the big picture issues here, let’s get the small stuff out of the way.

Avoid using ALL_CAPS for anything that isn’t a macro. The exception is single letter stuff like T and U; those are traditionally used for template parameters, and thus are safe. FUN is in a grey area that might be okay, or might not be. It would be better to use F or Fun.

T *arg is C-style. In C++, we prefer to put the type modifier with the type. So: T* arg.

You might want to mark the function noexcept, or at least conditionally noexcept. That could be important if releasing has to happen in a no-fail context.

You also might want to constrain the function to ensure that FUN can be called with a T*.

Okay, so the real point of this function is so you can write std::unique_ptr<T, Deleter<FunctionThatReleasesT>> for SDL types. That will allow you to write a smart pointer that destroys windows as std::unique_ptr<::SDL_Window, Deleter<::SDL_DestroyWindow>>. That’s okay, I guess.

Let me offer an alternative strategy. Instead, in your sdl namespace, create a non-template deleter type. In that type, create whatever deleters you want. For example:

namespace sdl {

struct default_releaser
{
    constexpr auto operator()(::SDL_Window* p) const noexcept
    {
        ::SDL_DestroyWindow(p);
    }

    constexpr auto operator()(::SDL_Renderer* p) const noexcept
    {
        ::SDL_DestroyRenderer(p);
    }

    constexpr auto operator()(::SDL_Texture* p) const noexcept
    {
        ::SDL_DestroyTexture(p);
    }

    constexpr auto operator()(::SDL_Surface* p) const noexcept
    {
        ::SDL_FreeSurface(p);
    }

    // ... and so on ...
};

} // namespace sdl

Then use an alias for any smart pointer types you want:

namespace sdl {

template <typename T>
using unique_ptr = std::unique_ptr<T, default_releaser>;

} // namespace sdl

Now you no longer need to manually alias everything, which is both tedious and introduces problems. In other words, this:

    using Window = std::unique_ptr<SDL_Window, Deleter<SDL_DestroyWindow>>;
    // ... [snip] ...
    using WindowSurface = std::unique_ptr<SDL_Surface, NoDeleter>;
    // ... etc.

… is no longer necessary, or worthwhile. A smart window pointer is just sdl::unique_ptr<SDL_Window>. Short and sweet, and it doesn’t hide important information, like that it’s a unique_ptr (Window is just opaque, on its own).

The other benefit of having that deleter is that if anyone ever wants to do any manual resource management, the tool is obvious. They don’t need to look up that the proper release function for SDL_Window is SDL_DestroyWindow(), while the release function for SDL_Surface is SDL_FreeSurface() or any other such nonsense, in order to write Deleter<::SDL_DestroyWindow>. sdl::default_releaser{}(whatever) just works, doing the default release behaviour for any SDL object.

    // Helper class to get a T* from a unique_ptr<T>, and itself from everything else
    template<typename T>
    class AsRaw {
    public:
        static constexpr auto call(auto &&t) {
            return t;
        }
    };

    template<typename T, typename D>
    class AsRaw<std::unique_ptr<T, D> &> {
    public:
        static constexpr auto call(std::unique_ptr<T, D> &t) {
            return t.get();
        }
    };

    template<typename T, typename D>
    class AsRaw<std::unique_ptr<T, D> &&> {
    public:
        static constexpr auto call(std::unique_ptr<T, D> &&t) {
            return t.get();
        }
    };

    template<typename T, typename D>
    class AsRaw<const std::unique_ptr<T, D> &> {
    public:
        static constexpr auto call(const std::unique_ptr<T, D> &t) {
            return t.get();
        }
    };

    template<typename T>
    auto as_raw(T &&t) {
        return AsRaw<T>::call(std::forward<T>(t));
    }

If the only purpose of AsRaw is to specialize as_raw()… then it is superfluous. You could simply do:

template <typename T>
struct is_smart_pointer : std::false_type {};
template <typename T, typename D>
struct is_smart_pointer<std::unique_ptr<T, D>> : std::true_type {};
// Detect other smart pointer types, if you like.

template <typename T>
inline constexpr auto is_smart_pointer_v = is_smart_pointer<T>::value;

template <typename T>
constexpr auto as_raw(T&& t) noexcept -> decltype(auto)
{
    if constexpr (is_smart_pointer_v<std::remove_cvref_t<T>)
        return std::forward<T>(t).get();
    else
        return std::forward<T>(t);
}

And, as a bonus, the smart pointer check trait is reusable.

    // Forward-declaration only, used for decltype to get the return type of a function pointer
    template<typename OUT, typename ...INS>
    OUT return_type(OUT (*)(INS...));

There is no need for this. We have std::invoke_result.

    /**
     * The wrapper
     * @tparam FUN The function to wrap
     * @tparam CHECK Whether to check the return value for errors (only used if the return type is not void)
     */
    template<auto FUN, bool CHECK = true>
    class Wrapper {
        using out_t = decltype(return_type(FUN));
    public:
        constexpr void operator()(auto &&... arguments) const requires(std::is_void_v<out_t>) {
            FUN(std::forward<decltype(as_raw(arguments))>(as_raw(arguments))...);
        }

        constexpr out_t operator()(auto &&... arguments) const requires (!CHECK &&
                                                                         !std::is_void_v<out_t>) {
            return FUN(std::forward<decltype(as_raw(arguments))>(as_raw(arguments))...);
        }

        constexpr out_t operator()(auto &&... arguments) const requires (CHECK &&
                                                                         std::is_pointer_v<out_t>) {
            auto res = FUN(std::forward<decltype(as_raw(arguments))>(as_raw(arguments))...);

            if (res == nullptr) {
                throw std::runtime_error(SDL_GetError());
            }

            return res;
        }

        constexpr out_t operator()(auto &&... arguments) const requires (CHECK &&
                                                                         std::is_integral_v<out_t> &&
                                                                         std::is_signed_v<out_t>) {
            auto res = FUN(std::forward<decltype(as_raw(arguments))>(as_raw(arguments))...);

            if (res < 0) {
                throw std::runtime_error(SDL_GetError());
            }

            return res;
        }
    };

There’s a lot we can do to simplify this wrapper.

First, there is no need for a specialization for returning void. These two functions:

        constexpr void operator()(auto &&... arguments) const requires(std::is_void_v<out_t>) {
            FUN(std::forward<decltype(as_raw(arguments))>(as_raw(arguments))...);
        }

        constexpr out_t operator()(auto &&... arguments) const requires (!CHECK &&
                                                                         !std::is_void_v<out_t>) {
            return FUN(std::forward<decltype(as_raw(arguments))>(as_raw(arguments))...);
        }

… reduce to this:

        constexpr auto operator()(auto&&... arguments) const {
            return FUN(std::forward<decltype(as_raw(arguments))>(as_raw(arguments))...);
        }

Next, forwarding the result of as_raw() but not the argument makes little sense. Since you’re passing everything through as_raw() anyway, it would make more sense to handle the forwarding there, as in forward_raw(), so:

        constexpr auto operator()(auto&&... arguments) const {
            return FUN(forward_raw<decltype(arguments)>(arguments)...);
        }

For checking, you could use two (or more) overloads, and switch based on the value of CHECK and the return type… or you could use a single overload like this:

template <auto F, bool Check = true>
class Wrapper
{
public:
    constexpr auto operator()(auto&&... args) const
    {
        if constexpr (Check)
            return _check(F(forward_raw<decltype(args)>(args)...));
        else
            return F(forward_raw<decltype(args)>(args)...);
    }

private:
    constexpr auto _check(auto* p)
    {
        if (not p)
            throw std::runtime_error(::SDL_GetError());
        return p;
    }

    constexpr auto _check(std::signed_integral auto i)
    {
        if (i < 0)
            throw std::runtime_error(::SDL_GetError());
        return i;
    }
};

But for more flexibility, instead of just taking Check as a bool flag, you could take a function, and use that to check. Have Check default to a dummy function or type to signal not to do a check, but if set to something else, call that as a function with the result the call to F(args...), and return that result (or throw if there’s an error).

    // Just a simple Context class to use RAII for init and quit
    class Context {
    public:
        Context() {
            sdl::init(SDL_INIT_VIDEO);
        }

        ~Context() {
            sdl::quit();
        }
    };

This won’t do in the general case.

First of all, it’s copyable. So:

auto c1 = sdl::Context();
auto c2 = c1;

// SDL_Quit() called twice.

Copying makes no sense here, and should be disabled.

Secondly, it’s movable. This is not a problem, since it should be movable, but you have to handle it carefully.

auto c1 = sdl::Context();
auto c2 = std::move(c1);

// SDL_Quit() called when c2 destructs... good.
// SDL_Quit() called when c1 destructs... bad.

I wrote a whole article on the universal resource pattern years ago, but the upshot is this: you need a flag in the class that determines whether to release on destruct. Set it to true after calling SDL_Init(), set it as false if move constructed, and don’t forget to swap when doing move assignment.

\$\endgroup\$
3
  • \$\begingroup\$ Thank you so much for the review! My final goal for this whole library is to first wrap these functions to have the benefits I mentioned, then there will be a second level which is more C++ idiomatic (classes...), followed by a game engine level and finally to create a simple 2D platform game. So, technically, this means that I am indeed looking for something that could nearly be ctrl+f/replaced into an existing code, but with some differences (for example, I never want to call a destroyer, they should all be automatic when, thus I will not create a wrapper around SDL_DestroyWindow). \$\endgroup\$
    – Alex
    Feb 26, 2023 at 13:10
  • \$\begingroup\$ You definitely raise interesting points when wrapping the return types, in my code I make SDL_GetWindowSurface return a unique_ptr with no deleter, but this indeed makes little sense as it is non-owning: it should just return the raw pointer. \$\endgroup\$
    – Alex
    Feb 26, 2023 at 13:16
  • 1
    \$\begingroup\$ Not wrapping the resource-releasing functions (SDL_Destroy*()) is probably good idea. It will make migration less automatic, but safer, because you will have to manually deal with transitioning from dumb resource handles to smart ones. Another option is to make better RAII wrappers from the start (ie, not just using unique_ptr). But I’m not sure, off the top of my head, how well that would work. \$\endgroup\$
    – indi
    Feb 27, 2023 at 11:05

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.