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

Unify the no_std and std impls of FontSystem #160

Merged
merged 3 commits into from
Aug 6, 2023

Conversation

notgull
Copy link
Contributor

@notgull notgull commented Jul 24, 2023

This would make it easier to add certain features (i.e. configurable font fallback) to both instances of the FontSystem.

This adds hashbrown and rustc-hash as dependencies for no_std.

@notgull
Copy link
Contributor Author

notgull commented Aug 5, 2023

@jackpot51 If you have time can you review this change?

@jackpot51 jackpot51 merged commit 0476d7c into pop-os:main Aug 6, 2023
2 checks passed
@notgull notgull deleted the changable-fallback branch August 6, 2023 22:08
@grovesNL
Copy link
Contributor

grovesNL commented Aug 9, 2023

@notgull won't these dependencies always be enabled on std? I thought they'd be gated under no_std

@notgull
Copy link
Contributor Author

notgull commented Aug 9, 2023

They do, but they're generally pretty lightweight dependencies.

@grovesNL
Copy link
Contributor

grovesNL commented Aug 9, 2023

Yeah that's fair, I'm using std but intentionally try to avoid hashbrown when targeting wasm for size, so it's not ideal to include them by default.

@notgull
Copy link
Contributor Author

notgull commented Aug 9, 2023

That's fair, but I'm not aware of a way to do this in stable Rust.

@grovesNL
Copy link
Contributor

grovesNL commented Aug 9, 2023

What prevents us from gating hashbrown and a hashmap alias to hashbrown under no_std? I'd be happy to submit a PR to adjust this but not sure if there's something I'm missing.

Edit: opened #168

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.

3 participants