fix(style_configurator): fix unsafe int to nk_bool pointer casts

[sleeptightAnsiC]

Fixes: #812

For more detailed explanation see the commit message or the linked issue. Also, if you're wondering if those are the only places affected by this issue, I really think there is nothing else left :)

[PavelSharp]

In fact, fixing this bug is not a trivial task. There are two possible strategies:

1. Preserve the existing API.
   In this case, priority is given to API stability, and existing user code is not affected. It may remain either working or broken. The main drawback is that users will intuitively expect show_buttons to be a boolean type, which can still lead to potential UB in similar cases.

2. Change the field type in nk_style_slider and nk_style_scrollbar to nk_bool.
   This would break the existing API and user code. However, I believe this is a worthwhile long-term investment, since user expectations would better match reality by reducing surprises and making the library more predictable.

[sleeptightAnsiC]

1. Preserve the existing API.

That wasn't really my intent.

I just wanted to fix this thing and move on. I only did it because I was investigating other issue related to nk_bool (#849) and found out this is the safest approach. This thing was working fine in the past and #185 broke it only because our CI can't really catch this (but maybe it should...) I tried to find other possible places affected by this and found none (explained in #812 (comment) )

The main drawback is that users will intuitively expect show_buttons to be a boolean type, which can still lead to potential UB in similar cases.

These days, if you find yourself writing a code with similar defect (aka passing int* into bool*), a compiler with half-decent warnings enabled should catch this. In fact, GCC 15 does this by default and reports as error. Proof here: https://godbolt.org/z/4Pjncdjqn And if your compiler cannot do this, well... you're screwed anyway, because those kinds of issues can happen all over the C code.

2. Change the field type in nk_style_slider and nk_style_scrollbar to nk_bool.

I would rather not do this.

Not only there are way more int fields with bool semantics like these two, there is simply no other use of nk_bool type anywhere in the style structs (EDIT: I think I found one somewhere). If you want these two changed, you should probably also change other similar fields elsewhere, and then go through each line that uses them. This in return will create even bigger mess and more bugs.

[sleeptightAnsiC]

2. Change the field type in nk_style_slider and nk_style_scrollbar to nk_bool.
This would break the existing API and user code. However, I believe this is a worthwhile long-term investment

there is simply no other use of nk_bool anywhere in the style structs. If you want these two changed, you should probably also change other similar fields elsewhere

Btw, I think I remember some people talking about serializing those structs. So there is some benefit of keeping them the same, and adding nk_bool would definitely break this use case. You can't really serialize bool safely, although serializing raw signed integer is not a good idea neither, it will still probably work on Win32 platforms. That might be why they haven't changed it back in #185 and why those were never changed from int to something like nk_int (or nk_int32). Changing any types in those structs feels like playing Russian Roulette. Don't do this, please...

[RobLoach]

Overall, I think this makes sense. Making show_buttons a nk_bool would also make sense to me, despite potentially breaking things 🤷

[sleeptightAnsiC]

@RobLoach latest force-push makes those casts explicit, just like you requested.

[PavelSharp]

@RobLoach As you can see, sleeptightAnsiC has already provided several arguments.
However, I guess the discussion above does not give enough attention to the fact that #185 introduced an inconsistency (defect).

At the moment, Nuklear contains several structures that use nk_bool as a data member, among them the style structure struct nk_style_chart, where show_markers is defined as nk_bool:

Nuklear/src/nuklear.h

Line 5228 in 25f33c8

nk_bool show_markers;

But in nk_style_slider and nk_style_scrollbar this was apparently overlooked:

Nuklear/src/nuklear.h

Line 5038 in 25f33c8

int show_buttons;

Nuklear/src/nuklear.h

Line 5134 in 25f33c8

int show_buttons;

Regarding API/ABI breakage, I believe the impact would be limited, since Nuklear defines nk_bool as int by default. This would only affect cases where nk_bool was explicitly redefined by the user.

[sleeptightAnsiC]

I guess the discussion above does not give enough attention to the fact that #185 introduced an inconsistency (defect).
At the moment, Nuklear contains several structures that nk_bool uses as a data member, among them the style structure struct nk_style_chart, where show_markers is defined as nk_bool:
But in nk_style_slider and nk_style_scrollbar this was apparently overlooked:

I believe, there are more issues like this one, not only in styling structs. I personally don't want to touch this (for all the reasons mentioned before). This can be fixed as another Issue/PR, but not this one.

[PavelSharp]

I believe, there are more issues like this one, not only in styling structs.

I may missing something, but I’m not entirely sure why this issue is being generalized to such a broad scope in the discussion.
As shown above, these two fields appear to be a concrete API defect, and fixing them would already improve API consistency. However, by broadening the problem to all similar cases, the conclusion naturally becomes that fixing it would be complex and risky. To me, this feels like a conceptual trap.

[sleeptightAnsiC]

these two fields appear to be a concrete API defect, and fixing them would already improve API consistency. However, by broadening the problem to all similar cases, the conclusion naturally becomes that fixing it would be complex and risky. To me, this feels like a conceptual trap.

Those were the only ones that we had the code coverage to catch. If someone can write a new faulty code that casts int* to bool* , then this can be caused by literally anything. The fact that it happened here wasn't caused by someone being confused by the API, but because #185 forgot to change this code path. That's all.

I may missing something, but I’m not entirely sure why this issue is being generalized to such a broad scope in the discussion.

Yes, you do:

Nuklear/src/nuklear.h

Line 5359 in 25f33c8

int cursor_visible;

Nuklear/src/nuklear.h

Line 4642 in 25f33c8

int use_clipping;

Nuklear/src/nuklear.h

Line 5741 in 25f33c8

int use_pool;

(and probably way more)

@PavelSharp This PR is only about fixing those two specific code paths. Either take it or leave it. If you want the struct methods changed, go open a new PR/Issue for it. I'm not going to do this myself (you already know my opinion on this).

[sleeptightAnsiC]

I just entered a new issue about all those int fields #891

The conversation above is irrelevant here. This PR only fixes the two existing code paths. Nothing else.

(also, if we would ever want to change those to nk_bool, the code in this PR will still be compatible with it)

[sleeptightAnsiC]

@RobLoach Fell free to either merge or reject this one. I will not be messing with those structs here.