Nuklear icon indicating copy to clipboard operation
Nuklear copied to clipboard

const correctness

Open Xeverous opened this issue 1 year ago • 2 comments

I'm wrapping this library into C++ (mostly for ease of use, reduced verbosity and increased code productivity; might release the code at some point) and I have noticed that const is rarely used.

Now, I know this is an immediate mode library and thus any widget-producing function can not have const nk_context* but many getter-only functions such as xxx_get_scroll for sure can take const context.

There are also minor issues in some specific functions, e.g.:

  • https://github.com/Immediate-Mode-UI/Nuklear/issues/700
  • nk_combo, nk_combobox and all related functions should take const char *const *, not const char ** as neither the strings nor the array is modified

I know very little internals of the library, but this issue is purely on the abstraction level so I should be able to make a PR. This would not be a breaking change. Any thoughts?

Xeverous avatar Oct 03 '24 15:10 Xeverous

I have noticed that a recent commit 0a28e3002be24dad4aa9263817fb96f3ad94a9b5 is basically a revert of older commit 2e4db87ed124f3c4eb098f7e501a2fc0aba4c974. I think this is a bad change. I know that standard functions use char** but it does not make sense to return a non-const pointer to a const string.

Xeverous avatar Oct 05 '24 19:10 Xeverous

struct nk_font* nk_font_atlas_add_from_memory(struct nk_font_atlas *atlas, void *memory, nk_size size, float height, const struct nk_font_config *config); takes a non-const pointer to the memory but looking at the implementation this memory is never modified (actually copied).

Same for nk_font_atlas_add_compressed.

Xeverous avatar Oct 06 '24 16:10 Xeverous

Is this still occuring?

RobLoach avatar Dec 12 '24 20:12 RobLoach

I'm not sure what you mean.

I listed few additional cases that I did not modify for simplicity of my PR - these obviously weren't adressed (unless someone else modified them in the meantime).

My PR was merged (https://github.com/Immediate-Mode-UI/Nuklear/pull/721) so normally I would close this issue but now I see that your own PR (https://github.com/Immediate-Mode-UI/Nuklear/pull/728) reverted my changes. The PR has barebone commit message and there is no comment anywhere why.

Can you tell me what's going on?

Xeverous avatar Dec 13 '24 18:12 Xeverous

Thanks for catching that. Perhaps a merge conflict occured. I'll have another look.

RobLoach avatar Dec 13 '24 19:12 RobLoach

Looks like there was a merge conflict, and I had brought the const attributes back in via a rebase over at https://github.com/Immediate-Mode-UI/Nuklear/commit/a315d25b4c33efa0e112cab9562460e785935a10 . Thanks for having a closer look! :+1:

RobLoach avatar Dec 15 '24 17:12 RobLoach

Good news. Thanks.

Xeverous avatar Dec 16 '24 10:12 Xeverous