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

fix to compile under msvc #21

Merged
merged 2 commits into from
Jul 3, 2024
Merged

fix to compile under msvc #21

merged 2 commits into from
Jul 3, 2024

Conversation

superwhiskers
Copy link
Contributor

the current code doesn't compile under msvc due to it not supporting c99 variable-length arrays. this should fix that and have equivalent behavior

@gvanas
Copy link
Contributor

gvanas commented Jul 2, 2024

Thanks, indeed, this should be fixed.

Still, I would prefer that we allocate a buffer of fixed size on the stack rather than through malloc. The maximum value capacityInBytes can reach is 64 (for KT256). What about defining a maxCapacityInBytes constant set to 64, setting the size of intermediate to that, and adding a check that capacityInBytes is not greater than maxCapacityInBytes?

@superwhiskers
Copy link
Contributor Author

malloc isn't being used here (it's only that _alloca is in the malloc.h header). this is using _alloca, which is behaviorally equivalent to what is done everywhere else (a variable length array). however, if you'd prefer, i can change it to do what you have described

@superwhiskers
Copy link
Contributor Author

to be abundantly clear, this is what i'm using: https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/alloca?view=msvc-170

however, now that i've taken a look at the documentation page, it may be better to use _malloca if we're to continue using variable length arrays. what you've suggested seems fine, the only thing i'd add is that if 64 truly is the maximum possible capacity value, that we may want to consider conditionally compiling it with an #ifndef NDEBUG to allow disabling that check since it shouldn't ever fail (this would be done automatically anyway if we used assert from assert.h).

@gvanas
Copy link
Contributor

gvanas commented Jul 3, 2024

OK, I understand now that what you proposed is still allocation on the stack.

This said, I still have a slight preference for the static allocation of 64 bytes for the sake of other compilers or embedded systems that may not like variable allocation. Also, I agree with putting the check under conditional compilation.

@superwhiskers
Copy link
Contributor Author

this should be good. let me know if you have any problems with it

@gvanas gvanas merged commit 0ced103 into XKCP:master Jul 3, 2024
4 checks passed
@gvanas
Copy link
Contributor

gvanas commented Jul 3, 2024

Thank you!

@superwhiskers superwhiskers deleted the msvc-fix branch July 3, 2024 09:27
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

Successfully merging this pull request may close these issues.

2 participants