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 #110

Merged
merged 17 commits into from
Aug 28, 2018
Merged

No std support #110

merged 17 commits into from
Aug 28, 2018

Conversation

mb64
Copy link
Contributor

@mb64 mb64 commented Jul 9, 2018

Adds support for using palette without the standard library, as discussed in issue #108.

Fixes #108.

One tiny question: because this would be in a future release (not 0.4.0), what version should be in the README example Cargo.toml for #![no_std]? I put 0.4, but was unsure about if it should be something different.

@Ogeon
Copy link
Owner

Ogeon commented Jul 9, 2018

Excellent! I will look closer at it tomorrow (I think), but until then, leave the version as 0.4. I have a script for incrementing it automatically throughout the project when I'm preparing a release, so I will make it find that one as well.

@Ogeon
Copy link
Owner

Ogeon commented Jul 9, 2018

Oh, and those test results are way off! I didn't think the soft floats would differ that much...

@mb64
Copy link
Contributor Author

mb64 commented Jul 9, 2018

I ran some tests, and mish::powf(x, 1./3.) returns NaN for x < 0.0852. However, the cbrt_f32 function seems OK. I'll push an update in a few minutes that changes it to use .cbrt().

@Ogeon
Copy link
Owner

Ogeon commented Jul 10, 2018

Didn't know qbrt existed, but that sounds like a better choice in any case. Looks like you have made some really good progress with making the tests pass, and the code looks good as well! I'm just wondering if the float functions follow any specification, seeing that they are not just plain wrappers.

@Ogeon
Copy link
Owner

Ogeon commented Jul 10, 2018

Maybe I should have checked your last commit... Then I would have known where most some of it came from. 😛

@Ogeon
Copy link
Owner

Ogeon commented Jul 31, 2018

Hi! I'm just checking in to see how it's going. If you are just taking a break or on vacation, or similar, I'll leave you to enjoy that. Otherwise let me know if you are getting stuck somewhere or if anything else came in the way.

Also, I wanted to check if you have followed the recent development over at https://github.com/japaric/libm. It has gotten a ton of help during the last few weeks, so it's almost feature complete. Would be nice to only have to depend on that one library.

@mb64
Copy link
Contributor Author

mb64 commented Jul 31, 2018

Sorry I haven't been working on this for a couple weeks -- I was of town for a while, and then just didn't get back to it. There are a few things left in this:

  • To be able to say that it depends on std, Cargo wants a feature name for serde that's not the same name as the library. Some options could be serialize, serializable, serialize_deserialize, json_formatting, etc. (Personally, I vote for serializable.)
  • Now that there's an actual community effort for math on no_std, I think that libm will eventually include any x86/arm hard float implementations, making the potential need for user-provided extern "C" implementations an even more niche use-case. I think it would probably be better (less surprising to users) to just use libm all the time (when std is disabled).
  • Unrelated to no_std, recent nightly's have "fixed" a thing with proc macros that palette_derive was using, so now there's lots of warnings (or errors with -Dwarnings) about macro hygiene, etc.

@Ogeon
Copy link
Owner

Ogeon commented Jul 31, 2018

Alright, no hurry. 🙂

  • So serde depends on std after all, or am I misunderstanding? In that case I would probably pick serializing as feature name.
  • Sounds good! Better to reduce the complexity now and add it back later if it turns out to be required.
  • Yep, see Replace the hidden module for derives with a hidden function #111. I would ideally want to fix it in a 0.4.1 release, but haven't gotten around to do it yet, as I have also been quite occupied elsewhere until about now.

@@ -7,8 +7,8 @@ fn lab_lch_green() {
let expect_lab = lch.into_lab();
let expect_lch = lab.into_lch();

assert_relative_eq!(lab, expect_lab, epsilon = 0.001);
assert_relative_eq!(lch, expect_lch, epsilon = 0.001);
assert_relative_eq!(lab, expect_lab, epsilon = 0.1);
Copy link
Owner

@Ogeon Ogeon Aug 1, 2018

Choose a reason for hiding this comment

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

Have you tried tightening these error margins again?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have -- it works fine 😃 thanks to libm.
I'll push those changes in a moment.

Copy link
Owner

Choose a reason for hiding this comment

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

Great! 😄

@mb64
Copy link
Contributor Author

mb64 commented Aug 1, 2018

I expect this CI run to fail, because of rust-lang/libm#4. I could only get it to work by running it with RUSTFLAGS=-Coverflow-checks=off. There are a few ways to deal with this:

  • Set RUSTFLAGS in the CI: this is unideal because it disables other overflow checks that you might want the CI to catch
  • Make lots of PR's to libm and wait for them to be released: this will take a while and potentially create merge conflicts with other changes, etc.
  • Both?

@Ogeon
Copy link
Owner

Ogeon commented Aug 1, 2018

I appreciate that you are already on the case with PRs! Hopefully we can get away with just a few of those, because I would really like to wait for it to be fixed in libm. To me, the alternative feels like just hiding the problem (and other potential problems, as you mention). Waiting is not a problem for me, but the conflicts could be annoying if the branches diverges too much. Maybe the whole thing could be split into two phases. The first one prepares for no_std by removing the current usages of std. The second one waits for the current issues in libm to be fixed and implements the final changes. I'm happy with the general implementation, so building it in debug mode is pretty much all that's left from my point of view.

What's your view on it? You are putting in a lot of work to get this through (which I once again appreciate a lot) and I don't want it to turn into a burden for you, rather than something enjoyable.

@mb64
Copy link
Contributor Author

mb64 commented Aug 1, 2018

The only necessary PR's to libm are for atan2 and sqrt; the others work fine. Here's an idea:

  • Add a commit to this PR that disables "really" using no_std based on a not-in-Cargo.toml feature (something like "libm_works"). This will make it at least mergeable.
  • Make another PR to Palette that undoes the temporary feature; wait to merge it until libm is OK.

@Ogeon
Copy link
Owner

Ogeon commented Aug 1, 2018

Sounds like a plan!

@Ogeon
Copy link
Owner

Ogeon commented Aug 1, 2018

By the way, I have a PR up with a quick fix for the hygiene warning and will make a release within the next day or so. Then I'll also merge those changes into master. 🙂

@Ogeon
Copy link
Owner

Ogeon commented Aug 6, 2018

The fix is in master now.

@mb64
Copy link
Contributor Author

mb64 commented Aug 27, 2018

I just pushed a merge from master -- it should pass CI now. The no_std float trait that uses libm is hidden behind a never-enabled feature flag.

I think it's OK to merge, waiting on reverting the float trait once the libm PR's are in and released.

@mb64
Copy link
Contributor Author

mb64 commented Aug 27, 2018

(As a side note -- the docs link in palette/README.md points to the 0.4.0 docs, not the 0.4.1 docs)

@Ogeon
Copy link
Owner

Ogeon commented Aug 28, 2018

Thanks! I saw that one of the travis jobs timed out, so I restarted it, but I'll merge it when it's all green. If you have your revert ready, you can upload it and open a PR that can stay open until it's time. Otherwise I'll make an issue for it so it's not forgotten.

(As a side note -- the docs link in palette/README.md points to the 0.4.0 docs, not the 0.4.1 docs)

Oh, you're right. It's supposed to be symlinked to the root README.md, but I must have messed something up...

@Ogeon
Copy link
Owner

Ogeon commented Aug 28, 2018

bors: r+

bors bot added a commit that referenced this pull request Aug 28, 2018
110: No std support r=Ogeon a=mb64

Adds support for using palette without the standard library, as discussed in issue #108.

Fixes #108.

One tiny question: because this would be in a future release (not 0.4.0), what version should be in the README example Cargo.toml for `#![no_std]`?  I put 0.4, but was unsure about if it should be something different.

Co-authored-by: Mark Barbone <mark.l.barbone@gmail.com>
@bors
Copy link
Contributor

bors bot commented Aug 28, 2018

Build succeeded

@bors bors bot merged commit c729c30 into Ogeon:master Aug 28, 2018
@evq evq mentioned this pull request Mar 25, 2019
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.

None yet

2 participants