suspicious static checks for sizeof(nk_bool)
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.
https://github.com/Immediate-Mode-UI/Nuklear/blob/fcd64f85e54b9d35eee13347748d70fa4d2e134e/src/nuklear_internal.h#L74-L78
https://github.com/Immediate-Mode-UI/Nuklear/blob/fcd64f85e54b9d35eee13347748d70fa4d2e134e/src/nuklear.h#L223-L227
Some reasoning why those assert even exist could be [snippet below] but I don't believe this justifies anything. https://github.com/Immediate-Mode-UI/Nuklear/blob/fcd64f85e54b9d35eee13347748d70fa4d2e134e/src/nuklear.h#L187-L194
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
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:
-
NK_STATIC_ASSERT(sizeof(nk_bool) == 4)vsNK_STATIC_ASSERT(sizeof(nk_bool) >= 2)This inconsistency is simply an oversight in the original PR. The>= 2check was added during review, but the corresponding assert innuklear_internal.hwas never updated to match it. -
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(whenNK_INCLUDE_STANDARD_BOOLis 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.
- 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.
- Improvement. Once the codebase has been validated to ensure that
nk_boolis used consistently and does not rely onint-specific properties, we may consider makingnk_booldefault tobool(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);
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).
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
inttonk_boolintroduced subtle bugs? Keeping the size the same asintwould 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.