Time To Start Moving To Stdbool
Closed, ResolvedPublic

Description

As is typical with a lot of older C code, Blender has its own ad-hoc definitions of a boolean type and associated false/true values. Most of the code uses the constants “FALSE” and “TRUE” defined in source/blender/blenlib/BLI_utildefines.h, while one file, intern/mikktspace/mikktspace.c, for some reason uses its own “TFALSE” and “TTRUE” definitions.

It is time to start moving to the convention laid down by C99, with a specific type called “bool” and values named “false” and ”true”, as defined in a standard header named “stdbool.h”. The enclosed patch causes any source file that includes BLI_utildefines.h to automatically have this type and associated values available. And just in case you’re trying to use an antiquated compiler that STILL isn’t C99-compliant after 13 years, there are compatibility workarounds (based on those from GNU Autoconf) to ensure the C99-compliant definitions still work.

I have also added an ‘advanced” CMake option called BOOL_COMPAT (which is on by default); turning this off removes the older “FALSE” and “TRUE” definitions, causing compile-time errors in all those places that have not yet been updated to use the stdbool definitions. This can be removed in future, once the stdbool cleanup is complete.

If this patch is accepted, it would be good to also lay down, as a matter of policy, that future additions to the Blender code should start using stdbool.

Morten Mikkelsen (mmikkelsen) added a comment.Via Old WorldDec 7 2012, 5:54 AM

As the comment in the header file clearly states it is specifically supposed to be a standalone and external set of files.
It's all explained in the header and if you search for blender tangent space in google.

hi, Id like to move to C99 eventually, but changed like this will cause many conflicts in branches and make many small changes in blender.
Since there are quite a few actively maintained branches that are preparing for merge, I think now isnt such a good time to make this change.

as you say in your say - this can be done gradually, but even in this case - we should agree on the bf-committers mailing list first.

Brecht Van Lommel (brecht) added a comment.Via Old WorldDec 17 2012, 4:52 PM

I think bool in C/C++ is commonly defined as int rather than char? It's slightly faster on 32 bit CPU's, though not sure if that's still true on 64 bit.

Lawrence D'Oliveiro (ldo) added a comment.Via Old WorldDec 20 2012, 12:49 AM

Thanks for not rejecting this patch submission out of hand. :)

Certainly I don’t want to disrupt things—at least not right away. This patch is intended to be able to be merged without having to change anything else. Then, once it’s in, we can at any time try doing builds with BOOL_COMPAT set to OFF, react with dismay at all the compile-time errors it triggers, and start patching them up at our leisure. This can take as long as you like, because BOOL_COMPAT is there to keep the backward compatibility. Once, and only once, all these errors have gone away, we can declare “mission accomplished” and get rid of the BOOL_COMPAT mechanism (and the obsolete TRUE and FALSE constants) altogether.

Notes...
on GCC 4.7x, 64bit linux sizeof(bool) == sizeof(char)

It would be nice if - for example, functions that return bool would give some compiler warning if returning/assigning values besides 0/1, (found a bug today that would have been avoided if this was so).

I checked and GCC doesn't do this (which is a shame IMHO), but not a reason to reject this patch. perhaps cppcheck or some other tool can be made to do it.

Campbell Barton (campbellbarton) added a comment.Via Old WorldJan 1 2013, 1:35 PM

Quick Q, whats '__bool_true_false_are_defined' added for?

Campbell Barton (campbellbarton) added a comment.Via Old WorldJan 1 2013, 1:48 PM

applied, r53480.

changed the cmake part to use check_c_source_runs(), so the source is inline.

Campbell Barton (campbellbarton) closed this task as "Resolved".Via Old WorldJan 1 2013, 1:48 PM
Lawrence D'Oliveiro (ldo) added a comment.Via Old WorldJan 1 2013, 10:39 PM

Cool! Thanks for including it. :)

Not sure what “__bool_true_false_are_defined” is for, but it’s defined in stdbool.h, and checked for in the original autoconf test, so it seems to be important.

Agreed about the lack of a diagnostic for out-of-range values. But they’re likely only important when the value is being used e.g. for array indexing, or direct comparisons with true and false (normally bad practice anyway), or in a switch statement. Presumably this is not already happening, otherwise the bugs would already be present. Changing to the standard “bool” type shouldn’t make any difference to this.

Add Comment