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

No_std with alloc. Compiles, tests pass #92

Merged
merged 1 commit into from
Nov 9, 2023

Conversation

hatmajster
Copy link
Contributor

@hatmajster hatmajster commented Nov 5, 2023

Changes many std usages to core, vectors from alloc through feature with hashbrown crate for HashMap/Set.
Honestly I don't know how much of it actually compiles with pure no_std, probably very little, but I just wished for no_std with alloc, hence this PR.
I have not used this in a real app/lib yet, but I hope the changes are trivial on its own. Sadly it introduces a bit of a mess in code.
I am open to any opinions, refactors, etc. Its definitely not perfectly done. Its okay if you don't want to merge this, I'll keep the fork, but it would be really cool if it did.

@hatmajster
Copy link
Contributor Author

Please note that any possible checks are still done for normal std. If you like I can create new workflow for no_std only - if its not a nuisance for you.

@Stoeoef
Copy link
Owner

Stoeoef commented Nov 6, 2023

Thanks for the PR! Adding no-std support is always a chore. But it should be feasible for spade and sounds like a good addition!
I'll look trough it in more detail within the week.

@hatmajster
Copy link
Contributor Author

I am doing this for few crates and honestly I just learned about this: https://crates.io/crates/no-std-compat -.- It simplifies a lot. If you want I can raise another PR just to compare and pick a better one.
It basically is a dummy std crate with core/alloc methods published as if they were in std. The only change needed is that every rs file needs to have prelude included (use std::prelude::v1::*;). After that, you simply use std like you would without no_std, though only a part is available of course.
I would say that the changes I made here are better if you would like to keep attention tono_std. If you would like to just have no_std as an option and still use normal std, this compatibility crate might be a better idea (though naturally you can always see no_std compatibility via some tests). But its just how I see it, I might be wrong

@Stoeoef
Copy link
Owner

Stoeoef commented Nov 7, 2023

I am doing this for few crates and honestly I just learned about this: https://crates.io/crates/no-std-compat -.- ...

That's an interesting crate - thanks for sharing! It's the first time I'm hearing from that. I think there's value in unifying how no-std crates are being built though I'm being hesitant to be an "early adopter" - the crate doesn't seem to be super wide spread as of now.
For spade I'd prefer to make it a no-std crate manually without adding an additional dependency - let's keep the approach in this PR for now!

Copy link
Owner

@Stoeoef Stoeoef left a comment

Choose a reason for hiding this comment

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

Review done!

I've found a lot of liking to the idea to be no-std by default. This will require a few changes (nothing major though?) and should be an overall simplification.

Once that's done this PR should be ready to go.

Cargo.toml Show resolved Hide resolved
src/delaunay_core/math.rs Outdated Show resolved Hide resolved
…or one Error trait, but can be turned off using features.
@Stoeoef
Copy link
Owner

Stoeoef commented Nov 9, 2023

Please note that any possible checks are still done for normal std. If you like I can create new workflow for no_std only - if its not a nuisance for you.

Yep, I plan to add that and some additional documentation.

@Stoeoef Stoeoef merged commit 5393872 into Stoeoef:master Nov 9, 2023
5 checks passed
@Stoeoef
Copy link
Owner

Stoeoef commented Nov 9, 2023

Feature is available with v2.3.0 on crates.io

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