fix no_std #127
fix no_std #127
Conversation
|
Cool, thank you for this! Is libm in a state where it can be used for the things we need now? That would be excellent! If this is all that's needed, it's definitely approved once the tests pass. As for the test results, that overflow seems strange. I hope it's not something too nasty... Otherwise, I guess they had to tweak some inference rules since last time it ran. |
|
hey @Ogeon - looks like the overflow is due to the longstanding issue rust-lang/libm#4 which was previously discussed in #110. so to your question - I suppose libm is not yet fixed. that libm issue is very interesting - it appears the overflow is intentional however it does not use IMO having working |
|
Ah, that one. I more or less agree with you, but it requires a workaround that will also work for the tests. I'm not a fan of the thought of disabling overflow checks for the whole library, for example, but maybe that's what we have to do... We can't wait for libm to move if we want this within a foreseeable future, so unless a better alternative has appeared since last time i checked, we have to do what we can with what we have. |
palette/src/convert.rs
Outdated
| @@ -391,7 +391,7 @@ where | |||
| /// 0.7353569830524495, | |||
| /// 0.5370987304831942, | |||
| /// ]; | |||
| /// let hsv = Bgr::from_raw_slice(&buffer)[1].into(); | |||
| /// let hsv: Hsv<palette::encoding::Srgb, _> = Bgr::from_raw_slice(&buffer)[1].into(); | |||
There was a problem hiding this comment.
Wasn't just Hsv enough? Srgb should be the default. Or was Srgb what it couldn't infer?
There was a problem hiding this comment.
yeah strangely enough, just Hsv didn't seem to be enough :(
There was a problem hiding this comment.
That's odd. Oh well. In that case can you add Srgb to the import at the top of the example? That would make this part a bit less noisy... I don't know if you tried it, but you may be able to skip the _ at least, unless I'm forgetting something.
There was a problem hiding this comment.
That would conflict with palette::Srgb imported above - do you have a preferred alias for the Srgb encoding? (e.g. eSrgb or similar?)
I have tried without the _ and that doesn't seem to be enough.
There was a problem hiding this comment.
Ugh, good job on me for not reading it all. It's also f64, not the default f32, so that's probably why _ has to be there.
Uhm... I usually prefer to spell it out if I can... You could put a self in the encoding import and at least shorten it down to encoding::Srgb, I guess. :) If that makes a difference. I'm just trying to put the focus on the right hand expression, but it's not that important.
There was a problem hiding this comment.
Oh, you can create an alias like type Hsv64 = Hsv<encoding::Srgb, f64>;!
|
What do you think about creating multiple travis jobs? One runs tests with |
|
There's one script (test_features.sh) that runs all the features one by one. I suppose that one can run with |
|
If we only run with |
|
I don't think it's going to be a problem, really. If you look at https://github.com/Ogeon/palette/blob/master/.travis.yml, it starts out by compiling and running with the default features. Then it runs the script and most of that script runs with |
|
Also, I'm afraid we may be maxing out on parallel runners already, so I don't think we will save any time on that unless they increased the limit. |
|
Sorry, I accidentally clicked the close and comment button. |
|
Ah gotcha! So we'd hit the default features in debug mode and individual features in release mode. That sounds good to me - will adjust |
|
I think that will cover enough of the cases to give a good enough result. The optional features are mostly just utilities. |
|
@Ogeon I think this should be good to go now |
|
Nice! I didn't have the time to check it until now, but it looks good! I'll try to get a release out this weekend. I would just like to check if the rest of the code is in a good state first. |
|
bors: r+ |
|
Oh, and once again, thanks for the help! |
Build succeeded |
fixes #125