Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

const correctness #716

Closed
Xeverous opened this issue Oct 3, 2024 · 7 comments
Closed

const correctness #716

Xeverous opened this issue Oct 3, 2024 · 7 comments

Comments

@Xeverous
Copy link
Contributor

Xeverous commented Oct 3, 2024

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.:

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
Copy link
Contributor Author

Xeverous commented Oct 5, 2024

I have noticed that a recent commit 0a28e30 is basically a revert of older commit 2e4db87. 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
Copy link
Contributor Author

Xeverous commented Oct 6, 2024

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.

@RobLoach
Copy link
Contributor

Is this still occuring?

@Xeverous
Copy link
Contributor Author

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 (#721) so normally I would close this issue but now I see that your own PR (#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?

@RobLoach
Copy link
Contributor

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

@RobLoach
Copy link
Contributor

Looks like there was a merge conflict, and I had brought the const attributes back in via a rebase over at a315d25 . Thanks for having a closer look! 👍

@Xeverous
Copy link
Contributor Author

Good news. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants