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 support #51

Merged
merged 1 commit into from
Dec 7, 2020
Merged

no_std support #51

merged 1 commit into from
Dec 7, 2020

Conversation

vadixidav
Copy link
Contributor

@vadixidav vadixidav commented Jun 20, 2020

This is the follow-up to #46. I fixed the build issue with the benchmark trying to use core when it is using std. I also added CI tests for no_std. If CI fails, I will make additional changes. I have not previously used Travis CI, so I don't know if what I wrote will work, but that is what CI is for 😃.

Closes #46

@vadixidav
Copy link
Contributor Author

@bluss Alright, got the CI working. Let me know if I need to do anything else.

@vadixidav
Copy link
Contributor Author

@bluss Pinging you again just to take a look and approve or suggest changes.

@vadixidav
Copy link
Contributor Author

@bluss Hey, just pinging you again. Let me know if everything looks good.

@bluss
Copy link
Owner

bluss commented Sep 14, 2020

Thanks, looks good! I think normally we'd bump to version 0.3 here since we have a Rust MSV bump. Can't that be avoided, though?

@vadixidav vadixidav force-pushed the no_std branch 2 times, most recently from 235b3f6 to 79a516b Compare September 14, 2020 18:07
@vadixidav
Copy link
Contributor Author

vadixidav commented Sep 14, 2020

@bluss I just went through the rounds and tried some stuff. It turns out to not be possible (to name alloc in stable): https://blog.rust-lang.org/2019/07/04/Rust-1.36.0.html

It was Rust 1.36 at which the alloc crate was stabilized. I think it makes the most sense to move on with this approach of bumping the version. I have reset the branch back to where it was. If you insist, we can maintain separate lines for all allocation regions in the code, though I am not 100% sure if that will work either and would have to experiment. Even if we did that, we would only be supporting stable on an older Rust, and no_std on 1.36, which is strange.

@bluss
Copy link
Owner

bluss commented Sep 14, 2020

We just need Rust 1.36 if the std feature is disabled. So we don't need to break existing users actually

@vadixidav
Copy link
Contributor Author

Alright, I will work on getting it to work with std on 1.28 and no_std on 1.36 simultaneously. I will mess around with the CI as well to ensure we test that.

@vadixidav
Copy link
Contributor Author

@bluss I believe it have it all working (locally at least). Do you mind if I also add two commits: one to reformat with rustfmt and one to fix all clippy lints (either by actually fixing them or by annotating them with allows)? I can do that in another PR as well if you like.

@bluss
Copy link
Owner

bluss commented Dec 2, 2020

Thanks for the fixes. I think formatting changes in a PR are detrimental since it's harder to review the actual changes that way, it is an impediment to easy inspection and approval.

Could you rebase and squash your changes? Then we can include them.

@vadixidav
Copy link
Contributor Author

Thanks for the fixes. I think formatting changes in a PR are detrimental since it's harder to review the actual changes that way, it is an impediment to easy inspection and approval.

Could you rebase and squash your changes? Then we can include them.

I will do this later today without the formatting.

@bluss
Copy link
Owner

bluss commented Dec 2, 2020

Don't worry about adding/subtracting formatting in particular, now I've read it. But let's reformat in a separate pr. :)

@vadixidav
Copy link
Contributor Author

Don't worry about adding/subtracting formatting in particular, now I've read it. But let's reformat in a separate pr. :)

Okay, so just for clarification, you want me to rebase this work (including its formatting changes) onto master, then you want a separate PR with a total reformat of the repository. Is this right? If so, I will get on that later, and if not, let me know.

@bluss
Copy link
Owner

bluss commented Dec 2, 2020

Sure. Main interest is in squashing together some of the commits and make it ready for merge. If the travis failure on 1.28 is unrelated we should ignore it now too.

Co-authored-by: Geordon Worley <vadixidav@gmail.com>
@vadixidav
Copy link
Contributor Author

@bluss Alright, this should be all good to go. It passed CI before I squashed it so it should be good. I had to just make a small modification to deal with CI.

@vadixidav
Copy link
Contributor Author

Also you can close the other PR #46.

@bluss
Copy link
Owner

bluss commented Dec 7, 2020

Thanks a lot!

@bluss bluss merged commit 97921e9 into bluss:master Dec 7, 2020
@bluss
Copy link
Owner

bluss commented Dec 7, 2020

In release 0.2.4

@vadixidav vadixidav deleted the no_std branch December 7, 2020 20:36
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