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

nim-bindings: Fix GCC-14 [-Wincompatible-pointer-types] issue #430

Merged
merged 4 commits into from
May 29, 2024

Conversation

jangko
Copy link
Contributor

@jangko jangko commented May 26, 2024

GCC-14 no longer automatically convert between pointer values of unrelated pointer types.

@jangko jangko marked this pull request as ready for review May 26, 2024 10:06
@asn-d6
Copy link
Contributor

asn-d6 commented May 26, 2024

Hey @jangko . Thanks for the PR!

I'm leaning towards ACKing it, but since it involves some low-level nim decisions, would it be possible to get another person of your team (i.e. another nim expert) to review your PR?

@jangko
Copy link
Contributor Author

jangko commented May 26, 2024

ping @tersec @arnetheduck @cheatfate

@jangko
Copy link
Contributor Author

jangko commented May 28, 2024

After discussion with other nimbus developers, we decide to take one step further by introducing breaking changes. Instead of Nim-array to C-struct conversion using pointer casting, we choose to use the C types directly in both kzg.nim and kzg_ex.nim and avoid aliasing rules violation altogether.

Further review and approval will come after we solve issues in other wrapper libraries to make sure no more surprises from gcc-14.

Copy link
Member

@jtraglia jtraglia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍 I don't see any obvious problems.

@jtraglia jtraglia merged commit d7511cd into ethereum:main May 29, 2024
38 checks passed
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.

4 participants