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

Make scalar code plain C #350

Merged
merged 1 commit into from
Apr 11, 2024
Merged

Make scalar code plain C #350

merged 1 commit into from
Apr 11, 2024

Conversation

Speykious
Copy link
Contributor

@Speykious Speykious commented Apr 11, 2024

I discovered that minifb uses and compiles non-Rust code for a few use cases, one of which is to make the image scaling functions optimized on Linux in all profiles.
I'm not keen on this approach, as it means that minifb is reliant on a separate compiler to build the project. As an example, I am using minifb for one of my projects, and someone who wanted to run it got a compiler error when compiling it on Linux. They didn't have a C++ compiler installed:
image

Ideally, I'd like to not depend on a C/C++ compiler on Linux.

One option is writing the scalar functions in Rust into their own crate, make minifb depend on that crate and ensure the profile for that dependency is always optimized:

[profile.dev.package.minifb-scalar]
opt-level = 3

although that means there is a second crate to be published on crates.io.

Another option is to just not optimize it, and instruct users of minifb to compile it with optimizations on all profiles:

[profile.dev.package.minifb]
opt-level = 3

although that means slower minifb if we want its debug info in our program. (There's also debug = true I guess?)

What are your thoughts on this matter?

This discussion aside, I made this PR to remove the need for a C++ compiler to be installed.
In my experience, it is much more likely for a C compiler to be installed on someone's Linux machine. I hope it's a bit useful :)

@Speykious
Copy link
Contributor Author

Speykious commented Apr 11, 2024

Shoot, I didn't realize I was out of date...
Edit: updated!

This removes the need for a C++ compiler to be installed.
It is much more likely for a C compiler to be installed on someone's Linux machine in comparison.
@emoon
Copy link
Owner

emoon commented Apr 11, 2024

Thanks for the PR!

The reason why it's implement the this way is because when the code was added there was no cargo support for having different opt-levels for other crates which has since been added. I think the change to making the code use C only (as it really is) is fine, but I'm not sure if it's worth splitting the code into a separate crate.

@emoon emoon merged commit 633e6cd into emoon:master Apr 11, 2024
4 checks passed
@Speykious
Copy link
Contributor Author

I see. Yeah that makes sense
Thanks for merging!

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