bad handling of bool's

[MyTDT-Mysoft]

you have parameters that use nk_bool, like for the nk_input_button, however you are handling those as int's instead... and in some places even comparing against "true" if (down==1 && ...) ... which is a bug and should be if (down && ...) (for the click on button release) and similar logic for the dragging of move/size)

so passing anything but 1 to that to cause those to fail.

[sleeptightAnsiC]

I'm pretty sure it's yet another leftover from the times when this library used ints as booleans (#185)

I image this will misbehave when someone makes a call by passing int variable and expecting it to convert to bool, like it would usually do with real bool. EDIT: some examples at https://godbolt.org/z/3c1hTcx91

Conditions such as foo==0 or foo!=0 should be fine, but those comparing to 1 are all wrong.

Thanks

[sleeptightAnsiC]

I tried searching with:

$ grep -rnE '\s*[!=]=\s*1[^0-9]' ./src/* ./demo/* ./example/*

and the only place I found was that one from nk_input_button:

Nuklear/src/nuklear_input.c

Line 91 in 8c9b376

if (down == 1 && id == NK_BUTTON_LEFT)

however, I totally see the same issue also happening when nk_bool is being stored and reused during comparison, like so:

Nuklear/src/nuklear_toggle.c

Lines 278 to 280 in 8c9b376

	old_val = *active;
	*active = nk_check_text(ctx, text, len, *active);
	return old_val != *active;

[MyTDT-Mysoft]

yeah i would use something like !old_val != !*active; for such cases... but it's usually better to just "booleanize" it with !!val or (val != 0) whenever setting those... as long the user can't set the stored value directly otherwise you have no choice but perform the "booleanize" (proper cast to boolean) when comparing....

[RobLoach]

Don't we have a nk_true and nk_false?

[MyTDT-Mysoft]

Don't we have a nk_true and nk_false?

nuklear does, but that does not change the fact that "true" is not a specific number, it's anything that is not false, so comparing two booleans can't be done directly unless they are 1 bit or unless the API that stores them will guarantee them to be equal, in freebasic with "true" being -1 instead of 1... i usually combine it with "and" to do some "functional speed up", when handling it at the asm level in x86 and x86_64, you have "setCC" instructions that set a byte to 1... "sar" and "sbb" ends with "-1" and simd comparsions do the "-1" version... (where -1 means all bits 1 independent of the size), i believe freebasic and C++ have "boolean" datatype that automatically normalize the "true" (altough i don't like to use them)

[rswinkle]

Don't we have a nk_true and nk_false?

nuklear does, but that does not change the fact that "true" is not a specific number, it's anything that is not false, so comparing two booleans can't be done directly unless they are 1 bit or unless the API that stores them will guarantee them to be equal, in freebasic with "true" being -1 instead of 1... i usually combine it with "and" to do some "functional speed up", when handling it at the asm level in x86 and x86_64, you have "setCC" instructions that set a byte to 1... "sar" and "sbb" ends with "-1" and simd comparsions do the "-1" version... (where -1 means all bits 1 independent of the size), i believe freebasic and C++ have "boolean" datatype that automatically normalize the "true" (altough i don't like to use them)

While we should certainly fix these, especially the explicit comparing against 1, it's not as bad as you make it out to be. Assuming the user actually uses nk_true and nk_false appropriately, which are defined as 1 and 0 respectively, everything works out fine (thanks to the way boolean expressions work in C/C++).

The bigger issue is if they use int rather than nk_bool around their code and then turn on INCLUDE_STANDARD_BOOL and then you get type problems and likely size mismatch issues. I'm guilty of this myself since I learned C89 first and never got out of the habit of using int for bool which is partly why I've been so hesitant/slow to switch it on and force the issue in my own projects. In any given project I use 2-4 different true/false pairs and it's terrible: GL_FALSE/GL_TRUE, SDL_FALSE/SDL_TRUE, nk_false/nk_true, and of course false/true. For the most part there's no issue since they're all 0/1 which matches the standard behavior but it's still ugly even when I try to match things to their lane.

Of course the fact that Nuklear is stuck on C89 means we never test INCLUDE_STANDARD_BOOL in our standard demos/examples which is probably half of why things like "== 1" have never been fixed.

[sleeptightAnsiC]

Don't we have a nk_true and nk_false?

Assuming the user actually uses nk_true and nk_false appropriately [...] everything works out fine

Huh? This seems to be missing the point?

Opinions aside. It's just not possible to do anything reasonable about this issue at the moment. Nuklear has no test coverage, not even all code under this repo is being compiled by CI, and the types are bonkers everywhere (e.g. #891) so we can't simply grep for all those booleans. Trying to go line-by-line in order to patch it is probably not a very good idea.

I'm not saying "no" but I hope you get my point.

[sleeptightAnsiC]

in some places even comparing against "true" if (down==1 && ...)

things like "== 1" have never been fixed.

Also, I'm going to repeat what I said in #936 (comment) I was able to find only ONE occurrence of == 1. If you've seen more, please, let me know. Maybe I missed something back when I grepped it.

[MyTDT-Mysoft]

yeah adjusting that one ==1 and adjusting the if (bool1 == bool2) should do fine, as i keep doing more stuff with nuklear i will let you guys know if i find another instance where this goes bonkers with those :)

[rswinkle]

Don't we have a nk_true and nk_false?

Yes, we do, which makes this less of a functional issue and more of a "good practice/style" issue, aside from the bool type size problem depending on whether stdbool is used.

Assuming the user actually uses nk_true and nk_false appropriately [...] everything works out fine

Huh? This seems to be missing the point?

How am I missing the point? A agree it should be changed.

Opinions aside.

It's not an opinion. As much as I agree it's ugly and should be changed, the behavior is perfectly well defined in both C and C++, guaranteed by the spec. The fact that all non-zero values are "true" is irrelevant if, as I said, the user does the semantically correct thing and only passes nk_false, nk_true, or a boolean expression to any nk_bool parameters (or explicitly int parameters that are actually booleans). They could also explicitly pass 0 and 1 and it would work too but we don't do that because it's worse to read, not because it wouldn't work.

comparing two booleans can't be done directly unless they are 1 bit or unless the API that stores them will guarantee them to be equal, in freebasic with "true" being -1 instead of 1

@MyTDT-Mysoft You mention freebasic. Are you using a Nuklear binding? Even if freebasic or other languages have different bool semantics it shouldn't have any effect on what is well defined in C/C++. If there's an issue in the translation at the API boundary I feel like that should be handled by the binding/port or the user of the binding.

[MyTDT-Mysoft]

Don't we have a nk_true and nk_false?

Yes, we do, which makes this less of a functional issue and more of a "good practice/style" issue, aside from the bool type size problem depending on whether stdbool is used.

Assuming the user actually uses nk_true and nk_false appropriately [...] everything works out fine

Huh? This seems to be missing the point?

How am I missing the point? A agree it should be changed.

Opinions aside.

It's not an opinion. As much as I agree it's ugly and should be changed, the behavior is perfectly well defined in both C and C++, guaranteed by the spec. The fact that all non-zero values are "true" is irrelevant if, as I said, the user does the semantically correct thing and only passes nk_false, nk_true, or a boolean expression to any nk_bool parameters (or explicitly int parameters that are actually booleans). They could also explicitly pass 0 and 1 and it would work too but we don't do that because it's worse to read, not because it wouldn't work.

comparing two booleans can't be done directly unless they are 1 bit or unless the API that stores them will guarantee them to be equal, in freebasic with "true" being -1 instead of 1

@MyTDT-Mysoft You mention freebasic. Are you using a Nuklear binding? Even if freebasic or other languages have different bool semantics it shouldn't have any effect on what is well defined in C/C++. If there's an issue in the translation at the API boundary I feel like that should be handled by the binding/port or the user of the binding.

well that's exactly the point here... C have defined "true" as anything that is not 0... so -1 is as true as 1... the function expects an "int" for C89 reasons, but the documentation explictly says it's a boolean... which can only be true if "anything that is not 0" means true, so my point was just to enforce that... and yes i'm using a semi-automatic translated .h header (just the imports, while the lib is compiled a static library in C)

[rswinkle]

well that's exactly the point here... C have defined "true" as anything that is not 0... so -1 is as true as 1... the function expects an "int" for C89 reasons, but the documentation explictly says it's a boolean... which can only be true if "anything that is not 0" means true, so my point was just to enforce that... and yes i'm using a semi-automatic translated .h header (just the imports, while the lib is compiled a static library in C)

You're misunderstanding things. There is a difference between what counts as true and what true is actually defined as. The former is "any non-zero value," but the latter which is what matters here, is always 1*

So again, if only boolean expressions or official boolean values (nk_false/nk_true or any other commonly used pair defined as 0 and 1) are used when an nk_bool or explicit-int-used-as-bool is asked for everything just works.

*technically for C++ and C23 it is effectively 1 because any conversion results in 0 or 1, even if officially true and false are distinct values for the special bool type, rather than just different names for 1 and 0.

EDIT: formatting