libsdl-org / SDL

Simple Directmedia Layer
https://libsdl.org
zlib License
9.85k stars 1.83k forks source link

SDL_bool discussion #8460

Closed slouken closed 12 months ago

slouken commented 12 months ago

Feedback from Discourse:

In SDL3: I noticed as I was setting up toggling between fullscreen that SDL_SetFullscreen() now takes a SDL_bool as an argument. My question is, why? If it took the traditional Uint32 flag I could xor it to toggle the flag. If it took a regular bool then I could say fullscreenFlag != fullscreenFlag;

SDL_bool has always been an odd duck. We want it to represent a true/false value, but it's not interchangeable with bool, std::bool or BOOL, which makes it awkward for developers.

How do people feel about making it just an int and define SDL_TRUE as 1 and SDL_FALSE as 0? @libsdl-org/a-team?

sezero commented 12 months ago

How do people feel about making it just an int and define SDL_TRUE as 1 and SDL_FALSE as 0? @libsdl-org/a-team?

I'm OK with that

Kontrabant commented 12 months ago

I'm not a fan of emulating boolean types as there's too many subtle things that can go wrong, so I'm in favor of getting rid of it.

slouken commented 12 months ago

I'm not a fan of emulating boolean types as there's too many subtle things that can go wrong, so I'm in favor of getting rid of it.

By getting rid of it, you mean redefining as int?

Kontrabant commented 12 months ago

Defining it as a signed int can have issues. @smcv laid them out in #7957, but the big footgun seems to be:

If it's going to be redefined, it should probably be unsigned to avoid the potential bitfield issue.

icculus commented 12 months ago

We have a separate bug for this topic in #7957 (EDIT: I had the wrong bug number here originally), but I suspect we're eventually going to land on changing this to an int.

I'm sympathetic to the unsigned concern, but also think it's a bad idea to do a "SomeArbitraryDataType x:1;" declaration; if you need a specific number of bits you should probably use a specific int type. Which is to say if this going to cause Visual Studio to complain about signed/unsigned comparisons everywhere, then I don't care about being unsigned for bitfields.

nomi-san commented 12 months ago

How about this case in SDL implementation?

void sdl3_impl(int cond) {
  if (cond) { /* works well */ }
  if (cond == SDL_TRUE) { /* ??? */ }
}

In C++, the SDL2's boolean works fine because it's an enum, the compiler will warn/err if we use other values.

smcv commented 12 months ago

This issue seems like essentially a duplicate of #7957.

Do I assume correctly that we are not willing to require C99, because we're still aiming to support some compiler (which one?) that doesn't implement a 24 year old C standard? Because if we can require C99, then requiring C99 <stdbool.h> and using bool would be the obvious representation. Libraries like GLib that have aimed not to break API/ABI since pre-2010 don't have that option, because switching to bool would be an incompatible change for them; but SDL3 is breaking API anyway, so there is no compatibility constraint and it might as well do what is most obvious in 2023.

If we can't require C99, then we have to fake booleans with integers (like GLib's gboolean) or with enums (like SDL2's SDL_bool); and if we do that, then yes, we have to be careful not to break invariants that are implied by documentation but not enforced by the compiler, like "booleans are always 0 or 1, but never -1 or 42". This is equally true regardless of whether the API says int, or unsigned, or a typedef for one of those like GLib's typedef int gboolean, or (at least in C) a typedef for enum like SDL2's typedef enum {SDL_FALSE = 0, SDL_TRUE = 1} SDL_bool.

I think there's value in having a typedef rather than using int or unsigned for boolean parameters directly, because that provides implicit documentation - if a function is declared as void setup_splines (SDL_bool reticulate) then I immediately know, without even reading its doc-comment, that it wants a boolean parameter, and not some sort of bitfield with flags, or a count of how many times to reticulate, or some other non-boolean thing.

In C++, the SDL2's boolean works fine because it's an enum, the compiler will warn/err if we use other values

But SDL isn't written in C++, so the implementation of SDL itself still has to be equally careful; and if you overrule the compiler with casts, I suspect most C++ environments will still let you get this wrong and store 42 in a variable that should have been a pseudo-boolean enum.

if this going to cause Visual Studio to complain about signed/unsigned comparisons everywhere

Why would you compare a boolean with not-a-boolean? And if you do, are you sure it's correct to do so, bearing in mind the traps that I described in https://github.com/libsdl-org/SDL/issues/7957#issuecomment-1632591533?

If compilers start warning us about doing int foo; ...; if (foo == SDL_TRUE) or SDL_bool cond = (flags & SHIFT|CTRL), then if anything that's a good thing: it stops us from falling into traps like those.

IMO, the operators that make sense for a boolean cond are:

but cond = foo, where foo is not boolean, is wrong: I would always want to write that as cond = !!foo or cond = (foo != 0) or similar. (But I recognise that this isn't everyone's coding style.)

smcv commented 12 months ago

If it took a regular bool then I could say fullscreenFlag != fullscreenFlag;

I assume the author of that comment must have meant fullscreenFlag = !fullscreenFlag (which you have to write out longhand, because != is not analogous to &= and |= because it already means something different).

If yes, my reading of the C standards is that that's equally valid to do with a bool, int or unsigned: the ! operator is well-defined for all types, not just booleans.

Compilers won't necessarily allow it for typedef enum { FALSE = 0, TRUE = 1 } SDL_bool, but arguably they should, because if you write

SDL_bool fullscreenFlag = ...;
fullscreenFlag = !fullscreenFlag;

that should (as far as I know) be equivalent to

SDL_bool fullscreenFlag = ...;
fullscreenFlag = (fullscreenFlag ? 0 : 1);

and both 0 and 1 are valid numeric values for a SDL_bool, so a competent compiler should be able to figure out statically that it's always OK and can't break any invariants.

icculus commented 12 months ago

I think there's value in having a typedef rather than using int or unsigned for boolean parameters

I don't think we're talking about removing the typedef completely, sorry if I made this unclear by saying "changing this to an int" ... I meant changing the typedef to an int, not removing it and favoring ints instead.

icculus commented 12 months ago

Why would you compare a boolean with not-a-boolean?

I mean, people will...? But now that you mention it, if that did trigger a compiler warning, maybe that's a good thing to discourage people doing that.

I was thinking more like some implicit thing that generates a boolean result, like if (mybool == (x < y)) ...if MSVC considers that second thing signed, that could get annoying.

But to be clear, I haven't tried this to see what Visual Studio will actually do. It might be fine.

icculus commented 12 months ago

Anyhow, let's bump back over to the original issue:

Duplicate of #7957