SDL3 API functions made fallible

[rom1v]

Many functions which returned void in SDL2 now return bool in SDL3.

• SDL_GetWindowSize()
• SDL_GetWindowPosition()
• SDL_RenderPresent()
• …

This means that the caller should check the result. This may have a lot of consequences downstream: client functions which were infallible should now be made fallible, and the error should be handled:

true on success or false on failure; call SDL_GetError() for more information.

But when looking at the SDL source code, it turns out that they may only return false due to CHECK_XXX_MAGIC:

SDL/src/video/SDL_video.c

Line 3205 in 5dab2c7

CHECK_WINDOW_MAGIC(window, false);

SDL/src/video/SDL_video.c

Lines 159 to 178 in 5dab2c7

	#define CHECK_WINDOW_MAGIC(window, result) \
	CHECK_PARAM(!_this) { \
	SDL_UninitializedVideo(); \
	return result; \
	} \
	CHECK_PARAM(!SDL_ObjectValid(window, SDL_OBJECT_TYPE_WINDOW)) { \
	SDL_SetError("Invalid window"); \
	return result; \
	}
	#define CHECK_DISPLAY_MAGIC(display, result) \
	CHECK_PARAM(!display) { \
	return result; \
	} \
	#define CHECK_WINDOW_NOT_POPUP(window, result) \
	CHECK_PARAM(SDL_WINDOW_IS_POPUP(window)) { \
	SDL_SetError("Operation invalid on popup windows"); \
	return result; \
	}

Therefore, currently, this bool only reports programming errors (for example the client calling display with a NULL display), so they should really be assertions, not errors to be handled by the caller (so the functions should still return void).

Note that the opposite change was made for SDL_LockMutex(), which were fallible in SDL2 but infallible in SDL3 (899eb0d) (which is a good thing).

[icculus]

We could add documentation that says "this function only fails if the object is invalid" without needing API changes, if we're willing to commit to that for various APIs.

I don't think asserts are the right approach, though, since most things are going to be using a release build of SDL that will silently fail...and the rest will be annoyed that SDL unnecessarily crashed the process (legit app bug or not).

[slouken]

I think we can leave this as-is for now, and revisit for SDL4.

[rom1v]

I think we can leave this as-is for now, and revisit for SDL4.

Yes, this is a breaking change, so it cannot be changed before a major release anyway.

We could add documentation that says "this function only fails if the object is invalid" without needing API changes

👍

For the migration to SDL3, I replaced all my calls with something like:

int w, h;
bool always_ok = SDL_GetWindowSize(mc->window, &w, &h);
(void) always_ok;
assert(always_ok);

It is only valid if it is documented that the function only fails if the object is invalid, so documenting this would be a great addition.

I don't think asserts are the right approach, though, since most things are going to be using a release build of SDL that will silently fail

That could be an assert_release() (like assert!() in Rust), which would basically call abort() on error.

and the rest will be annoyed that SDL unnecessarily crashed the process

IMO, the caller will typically not check the return value anyway if it may only fail when passing an incorrect value. By the way, the migration script from SDL2 does not add such a check.

If it fails and the return value is not checked:

int w, h;
SDL_GetWindowSize(window, &w, &h);
something(w, h);  // undefined behavior

then reading w and h is undefined behavior, so IMO crashing explicitly is better than undefined behavior.

Some of these checks are just best effort anyway, for example:

// simplified macro
#define CHECK_DISPLAY_MAGIC(display, result) \
    if (!display) {
        return result;
    }

It catches the programming error where the caller passes a NULL value, but not other cases, for example (because it's impossible to check) when it passes an initialized variable.

[maia-s]

I'd appreciate the documentation update if you want to commit to that. It'd let safe wrappers around SDL skip checking for the error case (because it's impossible to pass an invalid pointer in those)

[slouken]

Occasionally the internal implementation changes or the platform layer changes and the validation is no longer just checking the input pointer, so I don't think we'll commit to a documentation update like that.

[rom1v]

Occasionally the internal implementation changes or the platform layer changes and the validation is no longer just checking the input pointer, so I don't think we'll commit to a documentation update like that.

If these functions can "really" fail, then that's a big API change (which adds a lot of complexity), because then clients must always handle the cases where (for example) they cannot know the window size or position.

In particular, this means that any call to SDL_GetWindowSize(window, &w, &h) in a client project without checking the return value is a bug.

[slouken]

If these functions can "really" fail, then that's a big API change (which adds a lot of complexity), because then clients must always handle the cases where (for example) they cannot know the window size or position.

That's already true. Setting size and position is a request that can be denied or delayed by the window manager.

[slouken]

The short of it is, you can't rely on implementation details, as those can and will change over time. If it's important to you that a function not fail, then you should check the return value.

[rom1v]

The short of it is, you can't rely on implementation details

Of course, if a function can genuinely fail when used correctly, it must report an error, which must be checked.

However, returning an error instead of void comes with a cost. An infallible function is much easier to use than a fallible one, so it's better design to have functions that truly cannot fail (or can be made infallible) return void.

For example, it seems to me that making SDL_GetWindowSize() report an error only makes it more complex to use (causing a cascade of unnecessary consequences), without providing any benefit.