Add `SDL_NODISCARD` macro and conservatively apply it to `SDL_audio`

[vittorioromeo]

Add a SDL_NODISCARD macro that, when applied to a function, warns the user of that function if the return value is ignored. The presence of this macro prevents defects in programs caused by forgetting to check a function's error code return value, either by absentmindedness or because the function's name did not suggest that an error code would be returned.

As a proof of concept, the SDL_NODISCARD macro has been selectively applied to some functions in SDL_audio.

Description

The SDL_NODISCARD macro expands to, in order:

• [[nodiscard]] in C++17 and C23;
• otherwise, __attribute__((warn_unused_result)) for GCC and Clang;
• otherwise, _Check_return_ in MSVC;
• otherwise, expands to nothing.

The SDL_NODISCARD macro could theoretically be applied on any function that returns a value that the user must use or read. This would produce a warning any time the user clearly misuses a function, which massively decreases the likelihood of defects making it into actual shipped software.

The drawback is that pretty much every function in SDL would be decorated with SDL_NODISCARD, adding a bit of noise to SDL's headers.

As a starting point, I've decided to conservatively apply the SDL_NODISCARD macro using the same principles that I describe in my "Pragmatic Simplicity" CppCon 2022 talk:

• Functions that are named GetXXX something and clearly return a value are unlikely to be misused, therefore the SDL_NODISCARD macro was not applied there;

• Functions that return an error code and whose name does not suggest that a value is returned are prime candidates for a bug and maximize the noise-to-value ratio of SDL_NODISCARD, therfore the macro was applied there.

I am not against the idea of applying SDL_NODISCARD more liberally to ensure that even the most unlikely function misuses are caught at compile-time, but there is definitely a small readability impact on the codebase, and -- as explained in the talk -- if everything is marked SDL_NODISCARD, then the value and attention that the users give to such a macro is diluted.

If this PR is well-received, SDL_NODISCARD can be applied to other SDL modules in future PRs.

Existing Issue(s)

N/A

[0x1F9F1]

It's always tricky to decide when to use nodiscard.

Personally I wouldn't apply it to SDL_FlushAudioStream, SDL_ClearAudioStream, SDL_LockAudioStream, and SDL_UnlockAudioStream, since they only error if the stream is NULL (maybe they should return void instead?).

[vittorioromeo]

Personally I wouldn't apply it to SDL_FlushAudioStream, SDL_ClearAudioStream, SDL_LockAudioStream, and SDL_UnlockAudioStream, since they only error if the stream is NULL (maybe they should return void instead?).

I agree with that logic, in fact I did not apply SDL_NODISCARD to functions that explicitly mentioned that in their documentation, such as SDL_SetAudioStreamPutCallback which says:

\returns 0 on success, -1 on error. This only fails if stream is NULL.

If it is true that those functions you mentioned only return NULL if the given argument is NULL, then I can remove the SDL_NODISCARD and update the documentation.

[madebr]

I think, if we add this, that we should limit this to functions returning allocated memory and transferring ownership. Adding this to functions returning an error code should be opt-in imho. Also these kinds of macro are (slightly) related to #6337.

@icculus should verify whether wikiheaders.pl knows how to handle this.

[vittorioromeo]

I think, if we add this, that we should limit this to functions returning allocated memory and transferring ownership. Adding this to functions returning an error code should be opt-in imho.

That would be a more conservative and probably easier start compared to adding it to SDL_Audio, but I do disagree with your statement.

If a function can fail, especially if the failure can depend on some state outside of the function's parameters, the API should force the user to at least think about handling the error. SDL_NODISCARD is a clear indication that says "hey, just so you know, I could fail -- please check if I succeed, or if you don't really care just cast me to (void)".

Here's a table showing how using SDL_NODISCARD in APIs is the only way of preventing undesired runtime defects:

	Without SDL_NODISCARD	With SDL_NODISCARD
User cares about RC, but forgets to check	⚠️ RUNTIME BUG ⚠️	No extra syntax needed
User cares about RC, and remembers to check	No extra syntax needed	No extra syntax needed
User doesn't care about RC	No extra syntax needed	Just add (void)

[vittorioromeo]

Also these kinds of macro are (slightly) related to #6337.

I don't think adding a macro such as SDL_NODISCARD would be a problem at all. It should be extremely easy to parse, some languages can make good use of it, and if it's really a problem... just `sed -i 's/SDL_NODISCARD //g' filename' prior to the parsing step.

[vittorioromeo]

It's always tricky to decide when to use nodiscard.

Personally I wouldn't apply it to SDL_FlushAudioStream, SDL_ClearAudioStream, SDL_LockAudioStream, and SDL_UnlockAudioStream, since they only error if the stream is NULL (maybe they should return void instead?).

I've double-checked the implementation of those functions, removed SDL_NODISCARD, and changed their documentation to be consistent with other functions that only fail if the argument is null.

[slouken]

I don't mind adding SDL_NODISCARD, but I think we should only apply it to functions that return data that needs to be freed. Can you drop all the changes except the one in SDL_begin_code.h?

If we change that policy in the future, that's fine, but we should do it as a separate PR.

[ocornut]

It's probably nice for a few things, but I would be cautious adopting that sorts of stuff widely.

The beauty of libraries like SDL is to be able to make things fast and easily, and that includes giving programmers the liberty to take shortcuts, give them the responsibility to decide that "99% ok" is good enough in a given context. When the API gets too opinionated about what correctness means, you slowly raise the accessibility bar and lose a bunch of user, mostly newcomers. I suppose OP will disagree, but newcomers to programming are going to progress much faster in that "this needs to be 90-99% correct" environment than in a "this needs to be 100% correct NOW" environment.

By enforcing a change like that you are enforcing a paradigm shift, and then over time your friendly library for people wanting to get text and circles on screen has the sketch-and-craft energy of a Rust compiler (aka very little).

Not mentioned that this would generally be a widely breaking change, in the sense that someone building an old program will suddenly be overwhelmed with hundreds of warnings.

In theory adding (void) would be fine, and this is a decent intent marker, but you know how programming newbies have a tendency to bikesheed and over-complicate progress, and this would give them stronger signals to do so. This is the equivalent of enforcing -Weverything on everyone and everything including people who started programming five days ago and may not need to be spending their time thinking about non-important things like implicit signed-type conversions.

That said,

If that's technically possible, it may be fine to simply make this an opt-in include-time option to be enabled by users before including SDL.h. You could decide of e.g. three levels of nodiscard warnings, 0 being none, 1 being the default and only applying to some critical functions, 2+ being the zealous one. By defaulting to 1, SDL3 introduce basic awareness of some issues.

[vittorioromeo]

I don't mind adding SDL_NODISCARD, but I think we should only apply it to functions that return data that needs to be freed. Can you drop all the changes except the one in SDL_begin_code.h?

If we change that policy in the future, that's fine, but we should do it as a separate PR.

I am OK moving forward in this manner, I will remove everything but the changes to SDL_begin_code.h from this PR and we can have subsequent PRs to actually add the SDL_NODISCARD attribute to the codebase.

[slouken]

Merged, thanks!

[vittorioromeo]

The beauty of libraries like SDL is to be able to make things fast and easily,

"Fast and easily" isn't very valuable when the program doesn't work as expected, or when it crashes after it has been shipped due to an unexpected setup on the end user's machine that causes a detectable failure that wasn't reproducible on the developer's hardware.

and that includes giving programmers the liberty to take shortcuts, give them the responsibility to decide that "99% ok" is good enough in a given context

I completely agree with you that programmers should have this liberty, but as you said yourself it should be a "liberty" in a "given context", not the default!

Suppressing these warnings is trivial, if one wants to. For a single specific occurrence, a cast to (void) is sufficient: just 6 characters.

If one is so annoyed by these warnings and consistently decides not to check any error code in their program, they can also very easily just pass -Wno-unused-result, or even -DSDL_NODISCARD to their compiler as we do not override the macro if it was already defined.

Key points:

• Users aware that errors can happen but not interested in handling them already have the knowledge and tools to suppress these warnings.

• Users unaware that these errors can happen need [[nodiscard]] enabled by default to gain awareness and knowledge of the possible failure cases.

When the API gets too opinionated about what correctness means, you slowly raise the accessibility bar and lose a bunch of user, mostly newcomers.

You are considering newcomers coming from only a specific background. Any newcomer that needs/wants to use SDL and comes from a more modern or functional programming background will be turned off by the fact that completely preventable error cases were hidden because the API wasn't making them obvious through annotations or naming conventions.

I suppose OP will disagree, but newcomers to programming are going to progress much faster in that "this needs to be 90-99% correct" environment than in a "this needs to be 100% correct NOW" environment.

Sorry, but this is a strawman fallacy, this PR doesn't require code "to be 100% correct NOW". In fact, the code doesn't even have to handle the errors!

All it requires from the users is to be aware that an error might occur and decide whether they want to handle it or if they want to suppress it via (void) or a more aggressive project-wide compilation flag.

By enforcing a change like that you are enforcing a paradigm shift, and then over time your friendly library for people wanting to get text and circles on screen has the sketch-and-craft energy of a Rust compiler (aka very little).

And this one is a slippery slope fallacy. What I want to enforce is: APIs that can fail and end up causing real defects need to be explicit about it by either having a very obvious name or being marked as SDL_NODISCARD, so that the end user can consciously decide whether they want to handle the error or not.

A simple attribute is not going to turn SDL into a Rust codebase. No types are changed. No functionality nor semantics are changed.

A few, easily-ignorable warnings, that promote good practices and prevent bugs before a program is shipped is a service to all users. Prototyping and speed of development is easily retained by suppressing these warnings if really desired, even though I don't recommend it :)

Not mentioned that this would generally be a widely breaking change, in the sense that someone building an old program will suddenly be overwhelmed with hundreds of warnings.

This is a fair point, however -- as far as I understand -- SDL 3 is still under active development and far less popular than SDL2.

SDL3 also already introduces a massive amount of breaking changes from SDL2, and a very minor one that doesn't actually break existing program, but rather exposes potential weaknesses in them is nothing compared to the SDL2->SDL3 migration efforts.

In theory adding (void) would be fine, and this is a decent intent marker, but you know how programming newbies have a tendency to bikesheed and over-complicate progress, and this would give them stronger signals to do so.

I think you're putting your own programming style biases and your own expertise into the imaginary "programming newbie" you're thinking of. A programming newbie will more likely spend hours trying to figure out why something doesn't work and open up a poorly-written StackOverflow question only to be ridiculed when someone finally tells them that they forgot to check the return code a function.

In my professional and teaching career I've seen many such cases that could have been prevented by a simple [[nodiscard]].

This is the equivalent of enforcing -Weverything on everyone and everything [...]

This is absolutely not the equivalent of that, and your statement is a intellectually dishonest exaggeration of the goals and changes introduced by this PR.

If that's technically possible, it may be fine to simply make this an opt-in include-time option to be enabled by users before including SDL.h. You could decide of e.g. three levels of nodiscard warnings, 0 being none, 1 being the default and only applying to some critical functions, 2+ being the zealous one. By defaulting to 1, SDL3 introduce basic awareness of some issues.

It is technically possible to have multiple levels of [[nodiscard]] hidden behind different macros, and selectively enable one or more via defines. I'm not sure if that's a good idea though, but it's certainly something that can be considered.

It is a fact that such an approach would however be more complex, and deciding what is a "critical function" and what isn't could be pretty subjective.

I also don't feel really comfortable with a library letting users ignore imporant return values by default. I'd rather have this feature opt-out, so that, again:

• An experienced user who is aware of the consequences of ignoring errors can prototype as fast as they want;

• A new user who is unaware of those consequences or even of the errors themselves is prevented from introducing bugs in their program without them realizing.

[icculus]

Just add (void)

I strongly dislike this, fwiw.

[Sackzement]

I see nodiscard warnings as an equivalent to: "this function call makes no sense, if the return value is ignored". While a warning can be helpful for functions, returning an error value, the code would still be logical. Besides all that, I'm not a big fan of attaching the nodiscard attribute to so many SDL functions. It would clutter the headers quite a bit.

[vittorioromeo]

Just add (void)

I strongly dislike this, fwiw.

The best thing to do would be to handle or propagate the error. What exactly do you dislike about the (void) cast in the context where a programmer consciously decides to not handle an error?

If you dislike the syntax, it is also quite easy to create a more descriptive function:

void discard(int) { } 
// ...
discard(SDL_Whatever(/* ... */));

[vittorioromeo]

I see nodiscard warnings as an equivalent to: "this function call makes no sense, if the return value is ignored".

That's a very good use case for [[nodiscard]], but not the only one.

There's a range of functions that report failure via return value and a result via output parameter. The output parameter might be invalid if the return value reports an error.

[[nodiscard]] is also excellent on those functions because it prevents the programmer from mistakenly thinking that the output parameter is going to be in a valid state, which is only true if the function returned success.

While a warning can be helpful for functions, returning an error value, the code would still be logical.

That is only true if the happy path is taken :)

Besides all that, I'm not a big fan of attaching the nodiscard attribute to so many SDL functions. It would clutter the headers quite a bit.

13 characters of SDL_NODISCARD here and there would not make the headers more "cluttered" than they already are, and would provide valuable information about the behavior and semantics of the function to anyone browsing them. Case in point:

extern               DECLSPEC SDL_Window *SDLCALL SDL_CreateWindow(const char *title, int w, int h, SDL_WindowFlags flags);
// versus
extern SDL_NODISCARD DECLSPEC SDL_Window *SDLCALL SDL_CreateWindow(const char *title, int w, int h, SDL_WindowFlags flags);

[slouken]

I'm going to go ahead and make official policy on how SDL will use SDL_NODISCARD. Most SDL functions return either a result or an error code, and the expectation is that the user of the API will check the error code if they care about the result. As the library developers we don't need to enforce how application developers use the library. Additionally, functions with output parameters set them to a sane empty value when returning an error, so in general output parameter values are defined in both error and non-error cases.

So, the official policy is:

Functions which return a value that needs to be cleaned up in some way should be marked SDL_NODISCARD. All other functions will not, with the understanding that all SDL functions besides those cleanup functions returns some sort of result code that rigorous applications will check and handle.

@icculus, this means that normal usage of the API, with or without error checking, will not generate additional warnings.

If at some point we want to provide additional optional diagnostics, we can totally do that.

[ocornut]

Chef’s kiss: intellisense analyzer may be part of the conspiracy to make everyone’s life miserable:

[vittorioromeo]

Chef’s kiss: intellisense analyzer may be part of the conspiracy to make everyone’s life miserable:

Oh yes, let me include the entirety of <tuple> just to not write (void). 🤦‍♂️