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

Typos discovered while hamering on the repo #107

Closed
wants to merge 5 commits into from

Conversation

pirbo
Copy link
Contributor

@pirbo pirbo commented Mar 3, 2025

I've done some heavy work to avoid as much as possible the use of ctypes-foreign. Not sure, it is a good idea; I'll open a PR with the result where we'll can discuss whether it makes sense. Still, in the process, I've discovered a few glitches:

  • unused definitions
  • duplicates
  • functions SDL asks us to not use anymore
  • too generic types

that, I think, would be nice to change independently of the mega rewrite of the plumbing...

@dbuenzli
Copy link
Owner

dbuenzli commented Mar 3, 2025

This looks ok to me but please remove the commit that adds all these _ before variables. Also don't remove SDL_GetRevisionNumber if the function still exists in SDL. It has been bound, people may be using it, there's no need to break them.

@pirbo pirbo force-pushed the pirbo-structs-sideeffects branch from f59bf40 to 206164d Compare March 3, 2025 16:25
@pirbo
Copy link
Contributor Author

pirbo commented Mar 3, 2025

Done both

  • the _ thing was to please ocamlopt "unused variable" warning (which dune activates and even turns into an error but right completely unrelated)
  • yes SDL_GetRevisionNumber BUT if you try to use it statically (which we "do" once we generate the binding using stubs), your C compiler does emit a big warning that the function is deprecated and SDL devs want you to not use it anymore :)

int_v (AUDIO_S32LSB);
int_v (AUDIO_S32MSB);
int_v (AUDIO_S32SYS);
int_v (AUDIO_S32);
int_v (AUDIO_S32LSB);
int_v (AUDIO_F32LSB);
Copy link
Owner

Choose a reason for hiding this comment

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

Why are you removing these constants ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not. These are duplicates (look 5 lines above in this example)

Copy link
Owner

Choose a reason for hiding this comment

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

Ah yes thanks!

@dbuenzli
Copy link
Owner

dbuenzli commented Mar 3, 2025

Thanks they have been merged, could you please rebase your other PRs so that they are simpler to inspect.

@dbuenzli dbuenzli closed this Mar 3, 2025
@pirbo pirbo deleted the pirbo-structs-sideeffects branch March 3, 2025 17:00
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