SFML / CSFML

Official binding of SFML for C
https://www.sfml-dev.org
Other
349 stars 124 forks source link

Fail fast on invalid null arguments #239

Closed ChrisThrasher closed 2 months ago

ChrisThrasher commented 7 months ago

Macros like CSFML_CALL aim to be friendly to users who incorrectly pass NULL to a parameter that expects a non-null argument. In some cases this results in functions that return placeholder values or short-circuit their logic to avoid a null pointer deref. What I like about this approach is that it results in well-defined behavior in all build types.

What I propose instead is a simpler approach where we eliminate most macros in Internal.h in favor of a simpler CSFML_ASSERT macro that terminates the program (perhaps with a helpful log message) when invalid null arguments are found. I'd rather users be notified ASAP that their code is incorrect than CSFML make attempts to gracefully handle code that users should have never written in the first place.

ChrisThrasher commented 2 months ago

What I like about this approach is that it results in well-defined behavior in all build types.

This is not true. In Debug builds you get a placeholder value returned. In Release builds you get undefined behavior if certain pointer arguments are null. This lends more credence towards the idea of simply terminating upon invalid inputs with assert. Release builds will be no less safe than before while Debug builds will make it easier to actually debug your program.

Before:

bool sfMusic_isLooping(const sfMusic* music)
{
    CSFML_CALL_RETURN(music, isLooping(), false);
}

After:


bool sfMusic_isLooping(const sfMusic* music)
{
    assert(music); // or perhaps CSFML_ASSERT(music)
    return music->This.isLooping();
}

I find the latter solution easier to read.

I'm no longer as keen on the idea of a CSFML_ASSERT macro that does logging simply because I'd rather not do quite so much logging in the library. The failed assertion is good enough. An additional console log that repeats the same information isn't adding much value.