suspicious static checks for sizeof(nk_bool)

[sleeptightAnsiC]

If you try to provide your own NK_BOOL - either by defining it to C99 _Bool, C++ bool, or some small integer type like int8_t - there will be two static asserts that fail and prevent you to do so.

repro:

$ git clone https://github.com/Immediate-Mode-UI/Nuklear.git
$ cd Nuklear/
$ cat <<'EOF' >testcase.c
    #define NK_BOOL _Bool                                                                                                                                                                                                                                      
    #define NK_IMPLEMENTATION                                                                                                                                                                                                                                  
    #include "./nuklear.h"                                                                                                                                                                                                                                     
    int main(void) { return 0; }                                                                                                                                                                                                                               
EOF
$ cc testcase.c

result (CLICK ME)

In file included from testcase.c:3:
./nuklear.h:302:61: error: size of array ‘_dummy_array449’ is negative
  302 |   #define NK_STATIC_ASSERT(exp) typedef char NK_UNIQUE_NAME(_dummy_array)[(exp)?1:-1]
      |                                                             ^~~~~~~~~~~~
./nuklear.h:291:46: note: in definition of macro ‘NK_STRING_JOIN_IMMEDIATE’
  291 | #define NK_STRING_JOIN_IMMEDIATE(arg1, arg2) arg1 ## arg2
      |                                              ^~~~
./nuklear.h:293:36: note: in expansion of macro ‘NK_STRING_JOIN_DELAY’
  293 | #define NK_STRING_JOIN(arg1, arg2) NK_STRING_JOIN_DELAY(arg1, arg2)
      |                                    ^~~~~~~~~~~~~~~~~~~~
./nuklear.h:298:32: note: in expansion of macro ‘NK_STRING_JOIN’
  298 |   #define NK_UNIQUE_NAME(name) NK_STRING_JOIN(name,__LINE__)
      |                                ^~~~~~~~~~~~~~
./nuklear.h:302:46: note: in expansion of macro ‘NK_UNIQUE_NAME’
  302 |   #define NK_STATIC_ASSERT(exp) typedef char NK_UNIQUE_NAME(_dummy_array)[(exp)?1:-1]
      |                                              ^~~~~~~~~~~~~~
./nuklear.h:449:1: note: in expansion of macro ‘NK_STATIC_ASSERT’
  449 | NK_STATIC_ASSERT(sizeof(nk_bool) >= 2);
      | ^~~~~~~~~~~~~~~~
./nuklear.h:302:61: error: size of array ‘_dummy_array6128’ is negative
  302 |   #define NK_STATIC_ASSERT(exp) typedef char NK_UNIQUE_NAME(_dummy_array)[(exp)?1:-1]
      |                                                             ^~~~~~~~~~~~
./nuklear.h:291:46: note: in definition of macro ‘NK_STRING_JOIN_IMMEDIATE’
  291 | #define NK_STRING_JOIN_IMMEDIATE(arg1, arg2) arg1 ## arg2
      |                                              ^~~~
./nuklear.h:293:36: note: in expansion of macro ‘NK_STRING_JOIN_DELAY’
  293 | #define NK_STRING_JOIN(arg1, arg2) NK_STRING_JOIN_DELAY(arg1, arg2)
      |                                    ^~~~~~~~~~~~~~~~~~~~
./nuklear.h:298:32: note: in expansion of macro ‘NK_STRING_JOIN’
  298 |   #define NK_UNIQUE_NAME(name) NK_STRING_JOIN(name,__LINE__)
      |                                ^~~~~~~~~~~~~~
./nuklear.h:302:46: note: in expansion of macro ‘NK_UNIQUE_NAME’
  302 |   #define NK_STATIC_ASSERT(exp) typedef char NK_UNIQUE_NAME(_dummy_array)[(exp)?1:-1]
      |                                              ^~~~~~~~~~~~~~
./nuklear.h:6128:1: note: in expansion of macro ‘NK_STATIC_ASSERT’
 6128 | NK_STATIC_ASSERT(sizeof(nk_bool) == 4);
      | ^~~~~~~~~~~~~~~~

Those checks do not make much sense to me. Not only they differ from each other (1st one is >=2 and the 2nd one is ==4) but also they're conditional when pulling <stdbool> (which implies that NK_BOOL can be smaller than 2). Also, the check for sizeof(nk_bool) == sizeof(bool) seems irrelevant, because nk_bool is always the same as bool in this specific case.

Nuklear/src/nuklear_internal.h

Lines 74 to 78 in fcd64f8

	#ifdef NK_INCLUDE_STANDARD_BOOL
	NK_STATIC_ASSERT(sizeof(nk_bool) == sizeof(bool));
	#else
	NK_STATIC_ASSERT(sizeof(nk_bool) == 4);
	#endif

Nuklear/src/nuklear.h

Lines 223 to 227 in fcd64f8

	#ifdef NK_INCLUDE_STANDARD_BOOL
	NK_STATIC_ASSERT(sizeof(nk_bool) == sizeof(bool));
	#else
	NK_STATIC_ASSERT(sizeof(nk_bool) >= 2);
	#endif

Some reasoning why those assert even exist could be [snippet below] but I don't believe this justifies anything.

Nuklear/src/nuklear.h

Lines 187 to 194 in fcd64f8

	#ifndef NK_BOOL
	#ifdef NK_INCLUDE_STANDARD_BOOL
	#include <stdbool.h>
	#define NK_BOOL bool
	#else
	#define NK_BOOL int /**< could be char, use int for drop-in replacement backwards compatibility */
	#endif
	#endif

[sleeptightAnsiC]

Suggested fix: change the checks in src/nuklear.h to:

NK_STATIC_ASSERT(sizeof(nk_bool) >= 1); 
NK_STATIC_ASSERT(nk_true == (nk_bool)(1)); 
NK_STATIC_ASSERT(nk_false == (nk_bool)(0));
NK_STATIC_ASSERT(nk_false == !nk_true);
NK_STATIC_ASSERT(nk_false == !!nk_false);
NK_STATIC_ASSERT(nk_true == !!nk_true);

The checks from src/nuklear_internal.h can be probably removed since the public header already makes them.

I think, there are few more semantics that could be sanitized here, like (nk_bool)-1 but I don't believe they would be correct for integers.

EDIT: Compiler explorer link to play around with this: https://godbolt.org/z/ndKc3d84j

[PavelSharp]

Good afternoon. Historically, Nuklear used int as the boolean type. Later, in PR #185, support for a dedicated boolean type (nk_bool) was introduced. A large number of files were updated to replace int with nk_bool. Most of the behavior and constraints discussed here originate from that PR. Below is the relevant context:

1. NK_STATIC_ASSERT(sizeof(nk_bool) == 4) vs NK_STATIC_ASSERT(sizeof(nk_bool) >= 2)
   This inconsistency is simply an oversight in the original PR. The >= 2 check was added during review, but the corresponding assert in nuklear_internal.h was never updated to match it.

2. The conditional size checks:

#ifdef NK_INCLUDE_STANDARD_BOOL
    NK_STATIC_ASSERT(sizeof(nk_bool) == sizeof(bool));
#else
    NK_STATIC_ASSERT(sizeof(nk_bool) >= 2);
#endif

The intent here was to enforce the idea that nk_bool should either:

• be equivalent to bool (when NK_INCLUDE_STANDARD_BOOL is enabled), or
• behave like int (when using the default definition).

Using sizeof was seen as the simplest way to assert “equivalence” to one of these types. In other words, the original PR attempted to require that nk_bool is either bool or an int-sized type. One possible motivation for this was defensive: “What if the migration from int to nk_bool introduced subtle bugs? Keeping the size the same as int would reduce the risk issues.”

Ironically, this concern already surfaced later as issue #812.

My thoughts. To simplify this situation, we should first validate the codebase for correct usage of nk_bool, assuming its size may vary. Once that is done, the current size-based asserts can be safely relaxed or replaced.

3. What happens if both macros are defined?

#define NK_INCLUDE_STANDARD_BOOL
#define NK_BOOL some_type

In the current implementation, NK_BOOL takes precedence. This means NK_INCLUDE_STANDARD_BOOL is effectively ignored in that case.

4. Improvement. Once the codebase has been validated to ensure that nk_bool is used consistently and does not rely on int-specific properties, we may consider making nk_bool default to bool (from <stdbool.h>) whenever C99 or newer is detected.
   This would help prevent subtle issues such as the following:

bool s = false;
nk_checkbox_label(ctx, "checkbox", &s);

[sleeptightAnsiC]

The intent here was to enforce the idea that nk_bool should either:

• be equivalent to bool (when NK_INCLUDE_STANDARD_BOOL is enabled), or
• behave like int (when using the default definition).

Using sizeof was seen as the simplest way to assert “equivalence” to one of these types. In other words, the original PR attempted to require that nk_bool is either bool or an int-sized type. One possible motivation for this was defensive: “What if the migration from int to nk_bool introduced subtle bugs? Keeping the size the same as int would reduce the risk issues.”

Forcing int-like behavior and size does not make much sense if this symbol can be already set to _Bool by the library itself. It doesn't really prevent ABI breakage as the library clearly allows it and breaks it on its own. The assumption that library can set nk_bool to bool/_Bool, but user cannot, seems wrong.

No offense here, but I think these assertions were added somewhat "adhoc" (just whatever worked at the time).

[PavelSharp]

It’s fairly obvious that the current system of checks is somewhat contradictory, and I fully agree with that.

Nevertheless, in an attempt to partially justify the current implementation (since #185 was reviewed by two contributors), I said:

What if the migration from int to nk_bool introduced subtle bugs? Keeping the size the same as int would reduce the risk issues.

The key word here is reduce the risk. That is, if nuklear.h contains critical bugs due to the introduction of nk_bool, the current system of checks gives us only one way to trigger them - by defining NK_INCLUDE_STANDARD_BOOL.

Without the size checks, we would have an additional way to trigger a hypothetical bug - by defining something like #define NK_BOOL some_type (with sizeof < 2)

So the idea behind the current checks is to limit the ways a critical bug could occur, making it easier to reason about potential risks. That’s how I interpret the rationale for the current system of assertions.

No offense here

No need. We are all here to make the Nuklear better.