bluss / matrixmultiply

General matrix multiplication of f32 and f64 matrices in Rust. Supports matrices with general strides.
https://docs.rs/matrixmultiply/
Apache License 2.0
213 stars 25 forks source link

no_std support #51

Closed vadixidav closed 3 years ago

vadixidav commented 4 years ago

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 :smiley:.

Closes #46

vadixidav commented 4 years ago

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

vadixidav commented 4 years ago

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

vadixidav commented 4 years ago

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

bluss commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

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

vadixidav commented 4 years ago

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 commented 4 years ago

@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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

vadixidav commented 3 years ago

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 commented 3 years ago

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.

vadixidav commented 3 years ago

@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 commented 3 years ago

Also you can close the other PR #46.

bluss commented 3 years ago

Thanks a lot!

bluss commented 3 years ago

In release 0.2.4